-
Notifications
You must be signed in to change notification settings - Fork 10.5k
[ClangImporter] Support lazy member loading for import-as-member globals in extensions #71320
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
Conversation
There are two pieces of code that can add an extension’s members to a type’s lookup table: `NominalTypeDecl::prepareLookupTable()`, which initializes the lookup table before its first use, and `NominalTypeDecl::addedExtension()`, which only updates a lookup table that has already been initialized. The two functions ought to do the same things, but it’s difficult to predict which one will be exercised by a given test, and in practice our current unit tests only seem to use `NominalTypeDecl::prepareLookupTable()` in certain important situations. Factor the common code into a shared helper method, `MemberLookupTable::addExtension()`, to increase our confidence that both code paths will work correctly even if only one is hit by our unit tests. Fixes rdar://121479725.
This will help when ClangImporter::Impl::loadNamedMembers() starts having to handle a lot more import-as-members names, but it’s NFC in this commit.
Previously, the lazily-created extensions used for globals imported as members of a type used the lazy module loader, but `ClangImporter::Implementation::loadNamedMembers()` didn’t actually work for them. Other parts of the compiler instead contrived to avoid loading these members by name by forcing all members to load before any selective loading might occur. This commit modifies that code path to accommodate extensions with no matching clang node, which is how these are represented. With this change, other parts of the compiler can unconditionally use the `LazyMemberLoader` whenever it is present. There may be performance improvements from this change, but I don’t expect any functional changes.
@swift-ci please smoke test |
@swift-ci Please test compiler performance |
If this does actually let the compiler do less work, I'd expect to see:
|
Cool! |
No difference in source compatibility suite build failures vs. the baseline. Looking at the release data, we see the expected changes:
As well as a bunch of really nice knock-on effects:
One stat (Sema.LazyStoragePropertyRequest) regressed, but only in debug, and only by 1.34%. The topline Frontend.NumInstructionsExecuted and Frontend.MaxMallocUsage stats still decreased even in debug. 13% fewer imported decls!!!! |
Thank you for the thorough fix, Becca! |
A few people who've worked on this code before have looked at it and nobody's had any review comments, so I'm going to assume it's good to go. |
In rdar://123649082, a project failed to build because of the lazy import-as-member loading changes in swiftlang#71320. That project was configured in a way that broke modularization and the correct solution is to fix it, but out of an abundance of caution, add a `-disable-named-lazy-import-as-member-loading` frontend flag in case a project needs to temporarily restore the old behavior. As a bonus, this lets us write a test to verify that lazy import-as-member loading has positive performance impact.
In rdar://123649082, a project failed to build because of the lazy import-as-member loading changes in swiftlang#71320. That project was configured in a way that broke modularization and the correct solution is to fix it, but out of an abundance of caution, add a `-disable-named-lazy-import-as-member-loading` frontend flag in case a project needs to temporarily restore the old behavior. As a bonus, this lets us write a test to verify that lazy import-as-member loading has positive performance impact.
In rdar://123649082, a project failed to build because of the lazy import-as-member loading changes in swiftlang#71320. That project was configured in a way that broke modularization and the correct solution is to fix it, but out of an abundance of caution, add a `-disable-named-lazy-import-as-member-loading` frontend flag in case a project needs to temporarily restore the old behavior. As a bonus, this lets us write a test to verify that lazy import-as-member loading has positive performance impact.
In #71027, I addressed a cycle that could occur when loading import-as-member globals into an extension. However, that fix was minimal—it simply broke the cycle after it occurred. This PR is a more comprehensive fix that eliminates the cycle entirely by making import-as-member importing lazier.
The cycle was caused by the compiler forcing the extension to load its members immediately; it did that because the selective lazy member loading path (the
loadNamedMembers()
method) would crash for these specific extensions because they didn't have a clang node. This PR modifiesloadNamedMembers()
to work correctly, using the context parameter (instead of the absent clang node) to determine the submodule and skipping over lookup for the clang node's members. That allows us to modify name lookup to avoid forcing import-as-members extensions early. In addition to simplifying lazy member loader use sites, I expect this to reduce the number of members imported by ClangImporter, especially in code that usesNSNotification.Name
, which uses this mechanism for a massive number of members.The first commit also refactors the two code paths that add extension members to the member lookup table so that they share most of their code. It appears to be very difficult to write tests that are guaranteed to hit both of these, so in lieu of doing that, we can at least combine them to increase our confidence that both will work even if only one has been tested. This commit fixes rdar://121479725.