Partially revert 70d2ba016129e54dbb35947d5c44d8ae5f17e47c. #4083
Conversation
adrian-prantl
requested review from
kastiglione,
JDevlieghere and
jimingham
and removed request for
kastiglione
|
I'm still working on a testcase. Evidently, we need one... |
adrian-prantl
force-pushed
the
89836973-framework-paths
branch
from
90bf837 to
89b9ada
Compare
|
Added a test! |
|
@swift-ci test |
|
@swift-ci test macOS |
There was a problem hiding this comment.
Interesting. That's a nice twisty example.
You need to explain why the CoreAgentUnitTests is special, that just looks weird.
And I don't think you intend warmup_astcontexts to be used outside the handled_sdk_path test, but that isn't clear from the way you've structured the code.
| @@ -2144,7 +2173,26 @@ lldb::TypeSystemSP SwiftASTContext::CreateInstance( | |||
| handled_sdk_path = true; | |||
| } | |||
|
|
|||
| auto warmup_astcontexts = [&]() { | |||
There was a problem hiding this comment.
As a standalone routine, this would be an inefficient way to make sure that all the ASTContexts which we know about have been made, since once they have been made, you're spinning up a thread pool and adding a job to it for each module, when the call GetModuleSwiftASTContext which will just return the already made AST context.
The routine isn't a problem in this context, so far as I can tell, because its actual use is gated by handled_sdk_path, and so it only happens for the first module of the first big load where we discover the SDK. But the structure of the code doesn't make that clear.
You could make that clear by moving the definition of warmup_astcontexts into the if (!handled_sdk_path) scope so it can't be misused, or maybe just add a better comment.
You could make warmup_astcontexts cheap by having a GetModuleSwiftASTContext(bool can_build) which you call passing false before starting up the thread pool, so that you only set up the thread pool if there's work to do. But I don't think you intend this as a generally useful routine so that seems overkill.
There was a problem hiding this comment.
For context, the warmup code I put back in in this commit is identical to the code I removed a month ago.
There was a problem hiding this comment.
Okay. As long as this only gets run once it's okay. I had to think some to ensure that it wasn't, but since it's a temporary hack not many other people will have to warm up their brain pans over it so that's fine.
adrian-prantl
force-pushed
the
89836973-framework-paths
branch
from
89b9ada to
7f6714f
Compare
|
@swift-ci test |
| @@ -2144,7 +2173,26 @@ lldb::TypeSystemSP SwiftASTContext::CreateInstance( | |||
| handled_sdk_path = true; | |||
| } | |||
|
|
|||
| auto warmup_astcontexts = [&]() { | |||
There was a problem hiding this comment.
Okay. As long as this only gets run once it's okay. I had to think some to ensure that it wasn't, but since it's a temporary hack not many other people will have to warm up their brain pans over it so that's fine.
This fixes a regression in framework path discovery. It turns out that either ClangImporter or other Swift module loaders can discover additional framework search paths and add them to an existing CompilerInvocation on the fly. When 70d2ba0 switched to directly deserializing the compiler invocation it lost those additional late-discovered framework paths from the module contexts. rdar://89836973
adrian-prantl
force-pushed
the
89836973-framework-paths
branch
from
7f6714f to
cc20c75
Compare
|
Forgot to git-add main.swift |
|
@swift-ci test |
This fixes a regression in framework path discovery. It turns out that
either ClangImporter or other Swift module loaders can discover
additional framework search paths and add them to an existing
CompilerInvocation on the fly. When 70d2ba0 switched to directly
deserializing the compiler invocation it lost those additional
late-discovered framework paths from the module contexts.
rdar://89836973