Named lazy member loading 1/N #12429
Conversation
|
@swift-ci please test |
|
@swift-ci please smoke test compiler performance |
There was a problem hiding this comment.
Nice start! It's missing test cases, though. Make sure you include methods with NS_SWIFT_NAME annotations. Factory methods (class methods that return instancetype and start with the name of the type, like +[Foo fooWithValue:]) are another good test case because they get imported as both initializers and unavailable class methods.
(I think you should just pull the stats comments out into another PR. git worktree is great for scratch branches that you don't build locally.)
lib/AST/GenericSignatureBuilder.cpp
Outdated
| @@ -3216,6 +3216,10 @@ ConstraintResult GenericSignatureBuilder::expandConformanceRequirement( | |||
| } | |||
| } | |||
|
|
|||
| // Remaining logic is not relevant in imported-from-ObjC protocol cases. | |||
| if (proto->isObjC() && proto->hasClangNode()) | |||
| return ConstraintResult::Resolved; | |||
There was a problem hiding this comment.
Why is this specific to imported-from-ObjC protocols and not all @objc protocols?
There was a problem hiding this comment.
Well .. there was a testcase that seems to care (compiler crasher 28855-swift-genericsignaturebuilder...) but in general I think this code path is about improving diagnostics in cases that might credibly occur (according to Slava at least) and the cases we're cutting off are those where there's a typealias in a protocol. And that's a fair bit easier to write in swift as @objc protocol P { typealias ... (as in the testcase) than it is likely to arrive through a clang-import-as-member (which Slava at least implied was not worth worrying about here; of course your opinion may vary!)
There was a problem hiding this comment.
Ok if it regresses a crasher, sure. Maybe @DougGregor has ideas.
There was a problem hiding this comment.
I feel like whatever special things the GSB does with type aliases, it should not do them with objc protocols. Treat them like any other nested (nominal) type.
There was a problem hiding this comment.
I'm happy to just rewrite the crasher too, if this fix makes it no-longer-crash. I was just being a bit lazy. Preference?
lib/ClangImporter/ClangImporter.cpp
Outdated
|
|
||
| auto *CD = D->getClangDecl(); | ||
| if (!CD) | ||
| return true; |
There was a problem hiding this comment.
These are things you should assert.
There was a problem hiding this comment.
All 4 of them? DeclContext existing, ClangDecl existing, Submodule existing, and Lookuptable existing? I wasn't sure which of those are "sometimes not there" situations.
There was a problem hiding this comment.
Hm.
- A non-DeclContext can't have members.
- A non-Clang decl won't have the importer as its lazy loader.
- But a Clang decl from the bridging header won't have a Clang module (though it will have a lookup table).
- I think we have lookup tables for everything by now, so that actually ought to be safe to assert as well. We probably want to know if there isn't one.
lib/ClangImporter/ClangImporter.cpp
Outdated
| insertMembersAndAlternates(member, tmp); | ||
| for (auto *TD : tmp) { | ||
| if (auto *V = dyn_cast<ValueDecl>(TD)) { | ||
| Members.push_back(V); |
There was a problem hiding this comment.
You need to be filtering these by name. Remember that a single Clang declaration may get imported under multiple Swift names.
There was a problem hiding this comment.
I'm not sure I follow. I'm looking up by swift name, and I'm doing insertMembersAndAlternates to get the extra swift names it might be imported-as. Which names should I be filtering out? The alternates?
There was a problem hiding this comment.
No, the "alternates" are things like getters and setters for a property, or the original methods for Objective-C subscripts. I guess those do need to be filtered, but I was referring more to things like this:
- (void)foo NS_SWIFT_NAME(bar);This will show up in the lookup tables for both foo and bar, and will import the method under both names, but we only want to return the one with the matching name. You can take a look at the way top-level lookup does this in ClangImporter::Implementation::lookupValue.
graydon
force-pushed
the
named-lazy-member-loading
branch
from
a0ec2bd to
7e4ecfd
Compare
|
Ok, I've pulled the stat-comments out and landed as a separate commit, then fixed (hopefully) the name-filtering parts in clang importer, added some test infrastructure and a testcase against WebKit. Will try to make some more testcases against synthetic modules tomorrow, including the corner cases of ObjC Jordan mentioned, but .. is this a bit closer to viable? |
lib/ClangImporter/ClangImporter.cpp
Outdated
| TinyPtrVector<ValueDecl *> &Members) { | ||
|
|
||
| auto *DC = dyn_cast<DeclContext>(D); | ||
| assert(DC && "loadNamedMembers on a Decl that's not a DeclContext"); |
There was a problem hiding this comment.
This one you should just write cast. We're used to that, and you still get the backtrace.
lib/ClangImporter/ClangImporter.cpp
Outdated
| // - The decl is from a clang module, CMO is Some(M) for non-null | ||
| // M and we can use the table for that module. | ||
| // | ||
| // - The decl is from some other non-modular context, CMO is None. |
There was a problem hiding this comment.
This can't really happen, because Clang can't distinguish between the bridging header and the non-modular context. It looks like the actual case in which this returns None is when CD is a forward declaration and there's no definition, but in that case you wouldn't ask for its members.
graydon
force-pushed
the
named-lazy-member-loading
branch
from
7e4ecfd to
cecd7a6
Compare
| @@ -0,0 +1,33 @@ | |||
| #import <Foundation/Foundation.h> | |||
|
|
|||
| @protocol doer | |||
There was a problem hiding this comment.
Test style nitpick 1: Cocoa Objective-C capitalization rules mostly match Swift's, so protocols should be UpperCamelCase.
|
|
||
| @protocol doer | ||
|
|
||
| - (void) doSomeWork; |
There was a problem hiding this comment.
Test style nitpick 2: No space after the close paren.
|
|
||
| + (simpleDoer*) doer; | ||
|
|
||
| + (simpleDoer*) doerOfNoWork; |
There was a problem hiding this comment.
What are you testing with this? Was this class supposed to conform to the protocol you defined above?
There was a problem hiding this comment.
No (doing so actually causes the protocol to be entirely-imported because presumably conformance checking). I was hoping to add some members that could possibly (wrongly) collide in the member lookup table with the members we do want, to be sure we don't accidentally grab them. Maybe there's no chance of that happening.
| - (void) doSomeWorkWithSpeed:(int)s; | ||
|
|
||
| - (void) doSomeWorkWithSpeed:(int)s andThoroughness:(int)t | ||
| NS_SWIFT_NAME(doSomeWork(speed:thoroughness:)); |
There was a problem hiding this comment.
Test style nitpick 3: Despite mockery, Cocoa naming almost never uses "and"; without it, the Swift name you've picked would match the usual selector-splitting rules Swift already applies. I suggest a test cases where you change the base name:
- (void)doSomeWorkWithSpeed:(int)s thoroughness:(int)t
NS_SWIFT_NAME(doVeryImportantWork(speed:thoroughness:));and one where you only change argument names
- (void)doSomeWorkWithSpeed:(int)s alacrity:(int)a
NS_SWIFT_NAME(doSomeWork(speed:levelOfAlacrity:));and check that both the "before" and "after" names get imported.
There was a problem hiding this comment.
No mockery intended! I'm just .. continuing to absorb ObjC via testcases and inspection of headers. Fixed.
| // | ||
| // Check that with lazy named member loading, we're importing less. | ||
| // RUN: %target-swift-frontend -typecheck -enable-named-lazy-member-loading -stats-output-dir %t/stats-post %s | ||
| // RUN: %utils/process-stats-dir.py --evaluate 'NumTotalClangImportedEntities < 10' %t/stats-post |
There was a problem hiding this comment.
It seems brittle to depend on WebKit not to change. Can you make a fake test case instead?
There was a problem hiding this comment.
I .. guess? The synthetic testcase is the other one above (it includes a stats measurement). I was hoping to "check that it matters in the real world too" but I guess I can just omit this test.
utils/process-stats-dir.py
Outdated
| # are projected into python dicts (thus variables in the eval expr) named by | ||
| # the last identifier in the stat definition. This means you can evaluate | ||
| # things like 'NumIRInsts < 1000' or | ||
| # 'NumTypesValidated == NumTypesDeserialized' |
There was a problem hiding this comment.
This will definitely be useful!
graydon
force-pushed
the
named-lazy-member-loading
branch
2 times, most recently
from
95687e8 to
7068399
Compare
graydon
force-pushed
the
named-lazy-member-loading
branch
2 times, most recently
from
d9c9a3c to
c291e9f
Compare
|
Updated to reflect most-recent feedback. |
graydon
force-pushed
the
named-lazy-member-loading
branch
from
c291e9f to
d1b36b8
Compare
|
@swift-ci please test |
include/swift/AST/LazyResolver.h
Outdated
| /// \p Members is otherwise incomplete, due to implementation limitations | ||
| /// (eg. the implementation is unable to do named lookup at all, or within | ||
| /// the particular type of Decl that \p D is). | ||
| virtual bool |
There was a problem hiding this comment.
Would this API be cleaner if, instead of a bool result type and by-reference vector, it returned an Optional<TinyPtrVector<ValueDecl *>> ?
There was a problem hiding this comment.
I guess it means you can't get an "incomplete" result, where Members is partially filled but can't be completely filled, but that seems okay?
There was a problem hiding this comment.
Happy to change it to Optional, don't feel strongly. You prefer Optional here?
| @@ -5,5 +5,5 @@ | |||
| // See https://swift.org/LICENSE.txt for license information | |||
| // See https://swift.org/CONTRIBUTORS.txt for the list of Swift project authors | |||
|
|
|||
| // RUN: not --crash %target-swift-frontend %s -emit-ir | |||
| // RUN: not %target-swift-frontend %s -emit-ir | |||
There was a problem hiding this comment.
Unexpected like huh interesting, or unexpected like you'd prefer I detour into figuring out why it was crashing in the first place?
lib/AST/NameLookup.cpp
Outdated
| auto contextInfo = | ||
| ctx.getOrCreateLazyIterableContextData(this, | ||
| /*lazyLoader=*/nullptr); | ||
| if (contextInfo->loader->loadNamedMembers(this, name, |
There was a problem hiding this comment.
Hmmm, this won't be sufficient, because lookupDirect is supposed to return all of the members of the given name across the nominal type and all of its extensions, but the contextInfo is only associated with the nominal type declaration itself. At worst, we will have to poll several different sources to gather all of the declarations that match a given name:
- The Clang importer, which can answer this question by looking into all of the Swift name lookup tables,
- The Swift module loader, which can answer this question once we've added some name lookup tables for nominal types, and
- Walking the members of any extensions of the type that we parsed into the current module
We have to combine the results from all of these sources to get the right results from name lookup.
There was a problem hiding this comment.
Yes, those are reserved for phases 2/N, 3/N and so forth in this work series. This version literally just does ObjC @protocol members (and I mistakenly thought imported @protocols were not themselves extendable on the swift side). I'll add a check for "not extended by swift" for this variant; will return to this when we handle the swift side (i.e. have extended swiftmodules with member lookup tables too).
| // | ||
| // Check that with lazy named member loading, we're importing less. | ||
| // RUN: %target-swift-frontend -typecheck -I %S/Inputs/NamedLazyMembers -enable-named-lazy-member-loading -stats-output-dir %t/stats-post %s | ||
| // RUN: %utils/process-stats-dir.py --evaluate 'NumTotalClangImportedEntities < 15' %t/stats-post |
graydon
force-pushed
the
named-lazy-member-loading
branch
from
d1b36b8 to
cb1c852
Compare
|
@swift-ci please smoke test compiler performance |
|
@DougGregor let me know if this looks a bit more-right? |
|
@swift-ci please test |
|
Build failed |
|
Build failed |
This is an initial grab-bag of work related to reducing the number of codepaths in the frontend that eventually call
LazyMemberLoader::loadAllMembers, specifically as a consequence of doing a named member-lookup (as inNominalTypeDecl::lookupDirect).The goal here (grand plan provided by @DougGregor and @jrose-apple) is to support loading just the plausibly-same-named members of an
IterableDeclContextthat a given named member-lookup is after, rather than all the members.I'm starting with the lazy member loader in the clang importer (because it already has member names in the clang module lookuptables, which we can use for direct lookup). Within that, focusing on ObjC protocol members (because they're the simplest case, according to Jordan). And within that, at this stage it seems there are a bunch of non name-lookup-related paths that wind up triggering
loadAllMembersanyways, so this PR mostly just works on those.Commits included here:
-Xfrontend -enable-named-lazy-member-loadingoption flag, and debug loggingLazyMemberLoader::loadNamedMembers9c79770 and 1c306cc)
There are going to be a bunch more PRs in this series before it's done, but I figured I'd get review and integration started on the first set, to make sure I'm on the right path.