Skip to content

Re-apply "ModuleInterface: lock .swiftinterface while generating module cache" #29123

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Jan 10, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions include/swift/AST/DiagnosticsFrontend.def
Original file line number Diff line number Diff line change
Expand Up @@ -343,6 +343,12 @@ ERROR(unknown_forced_module_loading_mode,none,
"unknown value for SWIFT_FORCE_MODULE_LOADING variable: '%0'",
(StringRef))

REMARK(interface_file_lock_failure,none,
"could not acquire lock file for module interface '%0'", (StringRef))

REMARK(interface_file_lock_timed_out,none,
"timed out waiting to acquire lock file for module interface '%0'", (StringRef))

#ifndef DIAG_NO_UNDEF
# if defined(DIAG)
# undef DIAG
Expand Down
71 changes: 70 additions & 1 deletion lib/Frontend/ModuleInterfaceBuilder.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,7 @@
#include "llvm/Support/Errc.h"
#include "llvm/Support/Regex.h"
#include "llvm/Support/StringSaver.h"
#include "llvm/Support/LockFileManager.h"

using namespace swift;
using FileDependency = SerializationOptions::FileDependency;
Expand Down Expand Up @@ -240,7 +241,7 @@ bool ModuleInterfaceBuilder::collectDepsForSerialization(
return false;
}

bool ModuleInterfaceBuilder::buildSwiftModule(
bool ModuleInterfaceBuilder::buildSwiftModuleInternal(
StringRef OutPath, bool ShouldSerializeDeps,
std::unique_ptr<llvm::MemoryBuffer> *ModuleBuffer) {
bool SubError = false;
Expand Down Expand Up @@ -384,3 +385,71 @@ bool ModuleInterfaceBuilder::buildSwiftModule(
});
return !RunSuccess || SubError;
}

bool ModuleInterfaceBuilder::buildSwiftModule(StringRef OutPath,
bool ShouldSerializeDeps,
std::unique_ptr<llvm::MemoryBuffer> *ModuleBuffer,
llvm::function_ref<void()> RemarkRebuild) {

while (1) {
// Attempt to lock the interface file. Only one process is allowed to build
// module from the interface so we don't consume too much memory when multiple
// processes are doing the same.
// FIXME: We should surface the module building step to the build system so
// we don't need to synchronize here.
llvm::LockFileManager Locked(interfacePath);
switch (Locked) {
case llvm::LockFileManager::LFS_Error:{
// ModuleInterfaceBuilder takes care of correctness and locks are only
// necessary for performance. Fallback to building the module in case of any lock
// related errors.
if (RemarkRebuild) {
diags.diagnose(SourceLoc(), diag::interface_file_lock_failure,
interfacePath);
}
// Clear out any potential leftover.
Locked.unsafeRemoveLockFile();
LLVM_FALLTHROUGH;
}
case llvm::LockFileManager::LFS_Owned: {
if (RemarkRebuild) {
RemarkRebuild();
}
return buildSwiftModuleInternal(OutPath, ShouldSerializeDeps, ModuleBuffer);
}
case llvm::LockFileManager::LFS_Shared: {
// Someone else is responsible for building the module. Wait for them to
// finish.
switch (Locked.waitForUnlock()) {
case llvm::LockFileManager::Res_Success: {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is still going to cause stampedes. A lot of the interfaces take longer than the default timeout interval of 40 seconds to generate.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could change llvm side to take a parameter for waitForUnlock() to indicate the maximum waiting time. It doesn't fundamentally invalidate this approach.

// This process may have a different module output path. If the other
// process doesn't build the interface to this output path, we should try
// building ourselves.
auto bufferOrError = llvm::MemoryBuffer::getFile(OutPath);
if (!bufferOrError)
continue;
if (ModuleBuffer)
*ModuleBuffer = std::move(bufferOrError.get());
return false;
}
case llvm::LockFileManager::Res_OwnerDied: {
continue; // try again to get the lock.
}
case llvm::LockFileManager::Res_Timeout: {
// Since ModuleInterfaceBuilder takes care of correctness, we try waiting for
// another process to complete the build so swift does not do it done
// twice. If case of timeout, build it ourselves.
if (RemarkRebuild) {
diags.diagnose(SourceLoc(), diag::interface_file_lock_timed_out,
interfacePath);
}
// Clear the lock file so that future invocations can make progress.
Locked.unsafeRemoveLockFile();
continue;
}
}
break;
}
}
}
}
5 changes: 4 additions & 1 deletion lib/Frontend/ModuleInterfaceBuilder.h
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,8 @@ class ModuleInterfaceBuilder {
version::Version &Vers, llvm::StringSaver &SubArgSaver,
SmallVectorImpl<const char *> &SubArgs);

bool buildSwiftModuleInternal(StringRef OutPath, bool ShouldSerializeDeps,
std::unique_ptr<llvm::MemoryBuffer> *ModuleBuffer);
public:
ModuleInterfaceBuilder(SourceManager &sourceMgr, DiagnosticEngine &diags,
const SearchPathOptions &searchPathOpts,
Expand Down Expand Up @@ -102,7 +104,8 @@ class ModuleInterfaceBuilder {
}

bool buildSwiftModule(StringRef OutPath, bool ShouldSerializeDeps,
std::unique_ptr<llvm::MemoryBuffer> *ModuleBuffer);
std::unique_ptr<llvm::MemoryBuffer> *ModuleBuffer,
llvm::function_ref<void()> RemarkRebuild = nullptr);
};

} // end namespace swift
Expand Down
10 changes: 5 additions & 5 deletions lib/Frontend/ModuleInterfaceLoader.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -950,13 +950,11 @@ class ModuleInterfaceLoaderImpl {
std::unique_ptr<llvm::MemoryBuffer> moduleBuffer;

// We didn't discover a module corresponding to this interface.

// Diagnose that we didn't find a loadable module, if we were asked to.
if (remarkOnRebuildFromInterface) {
auto remarkRebuild = [&]() {
rebuildInfo.diagnose(ctx, diagnosticLoc, moduleName,
interfacePath);
}

};
// If we found an out-of-date .swiftmodule, we still want to add it as
// a dependency of the .swiftinterface. That way if it's updated, but
// the .swiftinterface remains the same, we invalidate the cache and
Expand All @@ -966,7 +964,9 @@ class ModuleInterfaceLoaderImpl {
builder.addExtraDependency(modulePath);

if (builder.buildSwiftModule(cachedOutputPath, /*shouldSerializeDeps*/true,
&moduleBuffer))
&moduleBuffer,
remarkOnRebuildFromInterface ? remarkRebuild:
llvm::function_ref<void()>()))
return std::make_error_code(std::errc::invalid_argument);

assert(moduleBuffer &&
Expand Down
13 changes: 13 additions & 0 deletions test/Driver/lock_interface.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
// RUN: %empty-directory(%t)
// RUN: echo 'public func foo() {}' > %t/Foo.swift
// RUN: %target-swift-frontend-typecheck -emit-module-interface-path %t/Foo.swiftinterface %t/Foo.swift -enable-library-evolution
// RUN: touch %t/main.swift %t/file-01.swift %t/file-02.swift %t/file-03.swift
// RUN: echo 'import Foo' > %t/file-01.swift
// RUN: echo 'import Foo' > %t/file-02.swift
// RUN: echo 'import Foo' > %t/file-03.swift
// RUN: %target-swiftc_driver -j20 %t/main.swift %t/file-01.swift %t/file-02.swift %t/file-03.swift -I %t -Xfrontend -Rmodule-interface-rebuild &> %t/result.txt
// RUN: %FileCheck %s -check-prefix=CHECK-REBUILD < %t/result.txt

// Ensure we only build Foo module once from the interface
// CHECK-REBUILD: rebuilding module 'Foo' from interface
// CHECK-REBUILD-NOT: rebuilding module 'Foo' from interface