Optimize Condition Resolution #6279

CodaFi · 2016-12-14T22:33:31Z

A bit of cleanup and a few optimizations around the recently-landed condition resolution stuff.

CodaFi · 2016-12-14T22:34:14Z

@swift-ci please smoke test.

CodaFi · 2016-12-14T22:37:58Z

include/swift/AST/Decl.h

+    /// later by Condition Resolution.
+    unsigned HasBeenResolved : 1;
+  };
+  enum { NumIfConfigDeclBits = NumDeclBits + 3 };


Off-by-one. Wonderful.

jrose-apple · 2016-12-14T22:41:18Z

lib/AST/ASTContext.cpp

@@ -1291,6 +1291,12 @@ ASTContext::getModule(ArrayRef<std::pair<Identifier, SourceLoc>> ModulePath) {
    return M;

  auto moduleID = ModulePath[0];
+  // Record this load request.  If we've seen it before and couldn't produce a
+  // loaded module it must have failed to load before.
+  if (!RequestedModuleLoads.insert(moduleID.first).second) {


Does it make sense to just use the existing LoadedModules map for this?

Does ModuleDecl * have a sensible sentinel value?

Identifier isn't the one that needs the sentinel; it's a map from Identifier to ModuleDecl *.

Ha. Well, that would go through a pointer's dense map info so... I'm not entirely sure what would work here.

llvm::MapVector<Identifier, ModuleDecl*> LoadedModules;

If moduleID.first is in the map, but the value is nullptr, then it must have failed to load. (This would require changing some of the other places that use LoadedModules too.)

CodaFi · 2016-12-15T21:20:42Z

I've withdrawn the parts of this pull request that deal with optimizing the module loading behavior. It turns out there's some trickiness there around loading modulemap-based modules. I'll try to take a crack at it later.

@swift-ci please smoke test.

jrose-apple · 2016-12-15T21:47:39Z

lib/Parse/ParseStmt.cpp

-          return
-            Context.getModule({ { argumentIdent, UDRE->getLoc() } }) != nullptr;
+
+          auto *M = Context.getModule({ { argumentIdent, UDRE->getLoc() } });


In retrospect this seems terrifying; loading a module has side effects even when it isn't imported. (Extensions still get hooked up, AnyObject dynamic lookup still finds things, operators still become visible.) All of those things are bugs, but we should probably put a canImport operation on ModuleLoader. (That'd be cheaper anyway if you didn't actually end up loading the thing.)

I agree. To what extent do we need to load a module to figure out if it is "importable?" It doesn't seem enough to search for it on disk, and it might depend on further modules that fail to load themselves.

CodaFi · 2016-12-15T22:29:32Z

OK, so there are three major cases to consider for the definition of 'Importable' here.

Clang Importer:

Modules still have to be imported by the CompilerInstance to check for importability, but the effects of the load can be hidden by specifying clang::Module::Hidden.

SerializedModuleLoader

Modules get run through (required) validation to report rich errors about failures. "Importable" here probably means "passes this validation", but that can be expensive for repeated requests.

SourceLoader

The hard one. Invokes the typechecker to get diagnostics in the dependent file, so probably need to add a new flag to the typechecker for a "dry run" or something to just see if the thing passes. Though, we might be able to get by with not running the type checker and just validating that we can load the file at all.

The subtext here is: does "I can import this" really mean "I can import this without error"?

slavapestov · 2016-12-15T22:32:24Z

@CodaFi Since you're working on this area, a few compiler_crashers are failing with various IfConfigStmt-related assertions. Might be an easy fix for you.

jrose-apple · 2016-12-15T22:53:23Z

FWIW, we don't actually care about SourceLoader. If we ever design a Python-like module system that lets us load from source, we'll build it from scratch to do some kind of subprocess compilation, not continue to make SourceLoader limp along.

For the ClangImporter, there may be Clang APIs we can use to determine if a module exists and can be imported without actually loading it, even as hidden. For SerializedModuleLoader, I think we can just do enough to detect that the module exists; this mechanism isn't supposed to be used for fallbacks like "oh, one of my dependencies is missing".

jrose-apple · 2016-12-15T22:55:33Z

At least, that's my understanding. @erica, without asking you to go read all the kinds of module loading we support, what behavior would you expect for a module that happens to exist on disk, but would fail some time in the loading process? It seems like checking for this in canImport would allow things to silently fail without anyone noticing.

slavapestov · 2016-12-19T02:12:12Z

@CodaFi Do you mind adding an entry to CHANGELOG.md to note that SE-0075 is now implemented?

erica · 2016-12-19T05:08:05Z

Here from tweet (http://twitter.com/CodaFi_/status/810709572956647424)

Putting kids to bed. My interpretation is if if the compiler emits "No such module (name)", it fails.

Email notifications from github seem borked and intermittent right now. Don't know why. If you don't see me respond to a name ping, please email me directly. @CodaFi @jrose-apple. I caught the tweet by accident.

I'll stick around for the next 30 minutes if you need me.

CodaFi · 2016-12-19T05:34:34Z

Thanks for taking the time.

Currently, that diagnostic is emitted late in the module loading phase. To take a fairly uncommon example in practice, but one relevant to something like the Swift Standard Library: What should the result checking a version of the Swift module built for an older compiler (currently something we diagnose as an error) be?

erica · 2016-12-19T05:57:43Z

@CodaFi What I believe you're asking is: how should if canImport(module) act in the presence of an invalid module. Logic suggests that a module that cannot be imported should fail this test.

Stepping back. The discussion for this proposal grew from platform differentiation, specifically for allowing module-dependencies to direct code. On iOS, watchOS, tvOS, you can use UIKit to perform identical tasks. Not all tasks but there's a basic core UIKit whose APIs are guaranteed to work across all three platforms. This test enables you to differentiate this code from code that run with Cocoa. Using modules lets you establish an API contract rather than testing for platform qualities.

You write:

What should the result checking a version of the Swift module built for an older compiler (currently something we diagnose as an error) be?

If you cannot import it for any reason, the API is unavailable, and the code should be excluded.

Will the module link, even with an older compiler?
Do modules have an inherent version number? (like SPM specifies?)
Is it necessary to specify the earliest acceptable version of Swift used to compile the module moving forward? e.g. #if canImport(ModuleName, swift: >=3.2), for some point where a 3.2 module would work with Swift 3.3?

erica · 2016-12-19T06:17:27Z

@jrose-apple @CodaFi

Ideally, a test for canImport that cannot import for any reason other than the module does not exist natively on that platform should emit a warning or error.

You can work around this in an extremely ugly way using Swift features that doesn't exist:

Test whether any conditional compilation that mentions canImport reaches #endif without any branches firing and emit a warning if that happens. If the conditions and the branches are not exhaustive, then there is a likely bug.
Allow developer-defined warnings to be emitted a la LLVM #warning/#error preprocessor directives. Developers add #warning/#error in an exhaustive else branch to override the compiler's default warning or to silence that warning because this is intentional design, with code that is guarded elsewhere.

karwa · 2016-12-19T18:05:59Z

I don't think it's right to require #canImport to be exhaustive (not sure if that's what you're saying). What about if you define some protocols in your library, and you want to expose some conformances via extensions to Foundation types, without requiring all supported platforms to support Foundation? e.g:

protocol MyProtocol {}

#if canImport(Foundation)
    // On platforms which don't support Foundation, this conformance isn't meaningful
    import Foundation
    extension Data: MyProtocol {}
#endif

I think that's a reasonably common use-case. You shouldn't have to have an 'else' block and manually silence a compiler warning.

jrose-apple · 2016-12-19T18:33:22Z

Ideally, a test for canImport that cannot import for any reason other than the module does not exist natively on that platform should emit a warning or error.

Hm. I agree with this in theory—you shouldn't be asking the question if the subsequent import is going to break—but at the same time I'd still like to not do all the work of loading a module just to be sure the error occurs. I guess it's rare enough that you'd want to check for the presence of a module but not load it that we don't need to worry for now, as long as the error still gets emitted.

I agree with @karwa that canImport should not require #else.

erica · 2016-12-19T19:12:27Z

@jrose-apple @karwa @CodaFi

Got to sleep on this overnight.

Hm. I agree with this in theory—you shouldn't be asking the question if the subsequent import is going to break—but at the same time I'd still like to not do all the work of loading a module just to be sure the error occurs. I guess it's rare enough that you'd want to check for the presence of a module but not load it that we don't need to worry for now, as long as the error still gets emitted.

If I understand this correctly, whether the import is successful or not, the test is for "would import RoseKit work, ignoring the suitability of RoseKit to link against the rest of the code"

The scenario I want to specifically avoid is this:

#if canImport(RoseKit)
    // something that isn't included because RoseKit was compiled for Swift 1.3
#endif

This should not silently exclude code because RoseKit is compiled for the wrong version of Swift. Something has to complain, I don't know where. If Swift "fails" here, I'd prefer it to fail by including the code and incurring failures later in the compilation process rather than excluding it and having that code be omitted without notice.

I'm thinking that so long as RoseKit is recognized as an available module, no matter whether it's reasonable to import it or not, the test will succeed if you have to allow/disallow earlier in the process than the point at which you decide whether the module is valid or invalid.

If the code is dependent on RoseKit, there are secondary mechanisms to catch the error.
If the code is not dependent on RoseKit (poor style, white shoes after labor day), then the mere presence of the module should be sufficient to allow it to compile early in the process.

Again, you guys all know the implementation details better than I do.

jrose-apple · 2016-12-19T19:18:14Z

Ah, yes, it definitely shouldn't silently skip the body of the #if. I would expect it to either emit an error and then skip the body (the good case), or silently process the body because we didn't get far enough into the import to detect an error. I think that means we're on the same page.

CodaFi · 2016-12-19T19:21:39Z

Fantastic, that should speed things up quite a bit. I'll update this pull request soon.

CodaFi · 2016-12-19T20:34:23Z

lib/ClangImporter/ClangImporter.cpp

+
+  // Now load the top-level module, so that we can check if the submodule
+  // exists without triggering a fatal error.
+  return (result != nullptr);


I'm unsure of whether this can all be replaced by

Impl.Instance->getPreprocessor().getHeaderSearchInfo().lookupModule(path)

This definitely seems interesting. @benlangmuir?

If not, I think we need to factor this out in some way to share code. Also, does this do the right thing for submodules?

canImport does not work with submodules at the moment. I can make it work and see what happens.

CodaFi · 2016-12-19T22:58:33Z

@swift-ci please smoke test.

CodaFi · 2016-12-22T23:28:35Z

@jrose-apple Is this still good to go?

jrose-apple · 2016-12-22T23:53:11Z

include/swift/AST/ASTContext.h

+  /// \brief Check whether the module with a given name can be imported without
+  /// importing it.
+  ///
+  /// Note that if errors occur during the module loading process this check may


That's not quite the condition we're going for. It's more "even if this check succeeds, errors may still occur if the module is eventually loaded", right?

jrose-apple · 2016-12-22T23:55:00Z

lib/ClangImporter/ClangImporter.cpp

+
+  // Now load the top-level module, so that we can check if the submodule
+  // exists without triggering a fatal error.
+  return (result != nullptr);


This definitely seems interesting. @benlangmuir?

If not, I think we need to factor this out in some way to share code. Also, does this do the right thing for submodules?

jrose-apple · 2016-12-22T23:56:10Z

lib/Serialization/SerializedModuleLoader.cpp

+      return false;
+    }
+  }
+  return (moduleInputBuffer != nullptr);


It's probably worth avoiding the construction of the llvm::MemoryBuffers. Maybe we should be passing pointers instead of references?

jrose-apple · 2016-12-22T23:57:19Z

test/Parse/ConditionalCompilation/can_import_sdk_idempotent.swift

+  class UIKitView : UIView {} // expected-error {{use of undeclared type 'UIView'}}
+#endif
+
+#if canImport(CoreGraphics)


We should have some tests for Swift modules that aren't overlays and for Objective-C modules that don't have overlays, because I don't trust overlays to do normal things.

Still applies.

A bit more detailed feedback for the more lazy approach.

jrose-apple · 2017-01-03T22:21:55Z

lib/ClangImporter/ClangImporter.cpp

+bool ClangImporter::canImportModule(std::pair<Identifier, SourceLoc> moduleID) {
+  // Look up the top-level module to see if it exists.
+  // FIXME: This only works with top-level modules.  To support submodules, this
+  // should be replaced by checking the result of `loadModule`.


I'm not sure this is the correct planned fix, but since we're only allowing users to check top-level modules for now it seems like an acceptable limitation. Maybe just delete the "To support submodules" part?

jrose-apple

I think all remaining comments are nitpicks. :-)

jrose-apple · 2017-01-03T22:22:20Z

lib/ClangImporter/ClangImporter.cpp

+  // should be replaced by checking the result of `loadModule`.
+  auto &clangHeaderSearch = Impl.getClangPreprocessor().getHeaderSearchInfo();
+  clang::Module *clangModule =
+    clangHeaderSearch.lookupModule(moduleID.first.str());


Still looking for confirmation from @benlangmuir that this is an acceptable way to check for the existence of a module without loading it.

jrose-apple · 2017-01-03T22:24:50Z

lib/Serialization/SerializedModuleLoader.cpp

  // Try to open the module file first.  If we fail, don't even look for the
  // module documentation file.
  Scratch.clear();
  llvm::sys::path::append(Scratch, DirName, ModuleFilename);
-  llvm::ErrorOr<std::unique_ptr<llvm::MemoryBuffer>> ModuleOrErr =
+  if (ModuleBuffer && ModuleDocBuffer) {


Nitpick: Let's flip this around and make it an early return.

jrose-apple · 2017-01-03T22:25:19Z

lib/Serialization/SerializedModuleLoader.cpp

+    if (ModuleDocOrErr)
+      *ModuleDocBuffer = std::move(ModuleDocOrErr.get());
+  } else {
+    return llvm::sys::fs::access(StringRef(Scratch.data(), Scratch.size()),


There's shorthand for this: llvm::sys::fs::exists

Trying to get a std::error_code return, not a boolean one.

jrose-apple · 2017-01-03T22:25:47Z

lib/Serialization/SerializedModuleLoader.cpp

+  // First see if we find it in the registered memory buffers.
+  if (!MemoryBuffers.empty()) {
+    // FIXME: Right now this works only with access paths of length 1.
+    // Once submodules are designed, this needs to support suffix


Let's not assume anything about how submodules might work.

jrose-apple · 2017-01-03T22:26:13Z

test/Parse/ConditionalCompilation/can_import_sdk_idempotent.swift

+  class UIKitView : UIView {} // expected-error {{use of undeclared type 'UIView'}}
+#endif
+
+#if canImport(CoreGraphics)


Still applies.

CodaFi · 2017-01-04T01:52:30Z

There. Barring comment by @benlangmuir

@swift-ci please smoke test.

CodaFi · 2017-01-04T03:33:02Z

@swift-ci please smoke test.

benlangmuir · 2017-01-04T19:42:08Z

lib/ClangImporter/ClangImporter.cpp

+  // FIXME: This only works with top-level modules.
+  auto &clangHeaderSearch = Impl.getClangPreprocessor().getHeaderSearchInfo();
+  clang::Module *clangModule =
+    clangHeaderSearch.lookupModule(moduleID.first.str());


Sorry for the late comment, but this will only tell you that the module map was found and parsed. The module may still be unavailable for a variety of reasons (e.g. it has an unfulfilled "requires" declaration). You should also call the following method from clang to check that the module makes sense to import:

/// \brief Determine whether this module is available for use within the /// current translation unit. /// /// \param LangOpts The language options used for the current /// translation unit. /// /// \param Target The target options used for the current translation unit. /// /// \param Req If this module is unavailable, this parameter /// will be set to one of the requirements that is not met for use of /// this module. bool isAvailable(const LangOptions &LangOpts, const TargetInfo &Target, Requirement &Req, UnresolvedHeaderDirective &MissingHeader) const;

I think you can just ignore the out parameters (Req and MissingHeader), since you're not trying to diagnose why the module isn't available.

Also, you can test this by adding a clang module whose module map contains e.g.:

module Foo { requires missing }

which should cause canImport(Foo) to fail. If you want a case where it succeeds, "requires swift" should do it.

The module loaders now have API to check whether a given module can be imported without importing the referenced module. This provides a significant speed boost to condition resolution and no longer introduces symbols from the referenced module into the current context without the user explicitly requesting it. The definition of ‘canImport’ does not necessarily mean that a full import without error is possible, merely that the path to the import is visible to the compiler and the module is loadable in some form or another. Note that this means this check is insufficient to guarantee that you are on one platform or another. For those kinds of checks, use ‘os(OSNAME)’.

CodaFi · 2017-01-05T08:38:52Z

@swift-ci please smoke test.

benlangmuir · 2017-01-05T16:39:51Z

LGTM

CodaFi · 2017-01-05T19:08:51Z

⛵️

This reverts the contents of swiftlang#5778 and replaces it with a far simpler implementation of condition resolution along with canImport. When combined with the optimizations in swiftlang#6279 we get the best of both worlds with a performance win and a simpler implementation.

* <swiftlang/swift#5778> * <swiftlang/swift#6279> * <swiftlang/swift#6547> * <swiftlang/swift#11613>

CodaFi commented Dec 14, 2016

View reviewed changes

jrose-apple reviewed Dec 14, 2016

View reviewed changes

CodaFi force-pushed the linker-loader-soldier-spy branch from 0d0dc31 to 5f6e545 Compare December 15, 2016 21:19

jrose-apple previously approved these changes Dec 15, 2016

View reviewed changes

CodaFi force-pushed the linker-loader-soldier-spy branch from 5f6e545 to 9c8683a Compare December 19, 2016 07:36

CodaFi force-pushed the linker-loader-soldier-spy branch from 9c8683a to 1ae4597 Compare December 19, 2016 20:32

CodaFi requested a review from jrose-apple December 19, 2016 20:33

CodaFi commented Dec 19, 2016

View reviewed changes

CodaFi force-pushed the linker-loader-soldier-spy branch 2 times, most recently from 08f9beb to 3b374c4 Compare December 19, 2016 22:57

jrose-apple reviewed Dec 22, 2016

View reviewed changes

CodaFi force-pushed the linker-loader-soldier-spy branch from 3b374c4 to 14f5251 Compare December 23, 2016 18:25

CodaFi force-pushed the linker-loader-soldier-spy branch 2 times, most recently from 0c7297c to 25f2cc5 Compare December 31, 2016 05:18

jrose-apple reviewed Jan 3, 2017

View reviewed changes

CodaFi force-pushed the linker-loader-soldier-spy branch from 25f2cc5 to 7c72f85 Compare January 4, 2017 01:51

CodaFi requested a review from benlangmuir January 4, 2017 01:52

Pack the bits for IfConfigDecls into Decl

7583c10

CodaFi force-pushed the linker-loader-soldier-spy branch from 7c72f85 to 5721a19 Compare January 4, 2017 03:32

CodaFi mentioned this pull request Jan 4, 2017

Simplify Condition Resolution #6547

Merged

benlangmuir reviewed Jan 4, 2017

View reviewed changes

CodaFi force-pushed the linker-loader-soldier-spy branch from 5721a19 to 399c029 Compare January 5, 2017 08:38

CodaFi merged commit dfa41d6 into swiftlang:master Jan 5, 2017

CodaFi deleted the linker-loader-soldier-spy branch January 5, 2017 19:08

benrimmington added a commit to swiftlang/swift-evolution that referenced this pull request Sep 3, 2017

[SE-0075] Implemented (Swift 4.1)

087d3fa

* <swiftlang/swift#5778> * <swiftlang/swift#6279> * <swiftlang/swift#6547> * <swiftlang/swift#11613>

CodaFi mentioned this pull request Dec 20, 2016

[SR-3438] Preprocessor directive not working when shadowing parameters #46026

Closed

Optimize Condition Resolution #6279

Optimize Condition Resolution #6279

Uh oh!

Conversation

CodaFi commented Dec 14, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CodaFi commented Dec 14, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CodaFi Dec 14, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CodaFi Dec 14, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CodaFi commented Dec 15, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CodaFi commented Dec 15, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

slavapestov commented Dec 15, 2016

Uh oh!

jrose-apple commented Dec 15, 2016

Uh oh!

jrose-apple commented Dec 15, 2016

Uh oh!

slavapestov commented Dec 19, 2016

Uh oh!

erica commented Dec 19, 2016

Uh oh!

CodaFi commented Dec 19, 2016

Uh oh!

erica commented Dec 19, 2016

Uh oh!

erica commented Dec 19, 2016

Uh oh!

karwa commented Dec 19, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jrose-apple commented Dec 19, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

erica commented Dec 19, 2016

Uh oh!

jrose-apple commented Dec 19, 2016

Uh oh!

CodaFi commented Dec 19, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

CodaFi commented Dec 19, 2016

Uh oh!

CodaFi commented Dec 22, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

CodaFi commented Dec 14, 2016 •

edited

Loading

CodaFi Dec 14, 2016 •

edited

Loading

CodaFi Dec 14, 2016 •

edited

Loading

CodaFi commented Dec 15, 2016 •

edited

Loading

CodaFi commented Dec 15, 2016 •

edited

Loading

karwa commented Dec 19, 2016 •

edited

Loading

jrose-apple commented Dec 19, 2016 •

edited

Loading

benlangmuir Jan 4, 2017 •

edited

Loading