Repository navigation
Conversation
Use native Clang category USRs and type/submodule identities for global-member extensions instead of materializing the entire imported module. Preserve Swift/fileprivate indexing and the recursive fallback. Add real-frontend identity and AST-allocation regressions. Fixes github#22771
Scope category USRs by their owning Clang module so same-named categories on the same type do not share an extension key. Cover the collision with a real-frontend regression.
|
I profiled a real Swift/Xcode build under CodeQL and confirmed that naming imported extensions was loading unrelated SDK declarations. I then validated the fix locally using real compiler ASTs and ran both extractors through full live rebuilds with the same source, compiler, CLI and queries, using source-only caching.
The runs with matching per-file declaration, body, call, resolved-call and direct local-flow measurements took 37m 33s → 28m 29s: about 24% less wall time. *One baseline had incomplete extraction. †The faster patched repeat had different extraction-success markers, resolved-call counts and diagnostics. Existing toolchain compatibility errors also remain. These results are therefore provisional, not a general coverage-equivalent speedup claim. AI assistance: GitHub Copilot assisted with implementation and validation. |
There was a problem hiding this comment.
🟡 Changes recommended
Global-member mangling still eagerly loads all members mapped to the same extension.
1 open finding
What changed in this PR
Optimizes Swift imported-extension mangling by deriving stable identities from Clang metadata instead of indexing entire modules.
Changes:
- Uses category USRs and Clang submodule names for extension identities.
- Adds regression coverage for allocation behavior, identity uniqueness, ordering, and late imports.
- Adds a macOS Bazel test harness.
| File | Description |
|---|---|
swift/extractor/mangler/SwiftMangler.cpp |
Implements Clang-backed extension identities. |
swift/extractor/mangler/tests/mangle.cpp |
Provides the native mangling test harness. |
swift/extractor/mangler/tests/test.py |
Exercises performance and identity behavior. |
swift/extractor/mangler/tests/BUILD.bazel |
Configures the macOS test target. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| LOG_WARNING("Unable to generate an imported category USR; using declaration indexes"); | ||
| } else if (!decl->getClangNode() && llvm::isa<swift::ClangModuleUnit>(decl->getDeclContext())) { | ||
| // The importer creates one global-member extension per nominal type and Clang submodule. | ||
| for (auto member : decl->getAllMembers()) { |
|
Note that the test you added times out: |
|
Due to the lack of explanation/reasoning it is impossible at this point to understand whether the change here is correct. In fact, unless there are additional preconditions that are satisfied by clang modules that are not satisfied by pure Swift modules, this looks incorrect, because the indexing mechanism for extensions is now completely bypassed in some cases, and in general that mechanism is critical for generation unique database keys for different extensions. |
|
@jketema are you cool with me jamming a bit more on it? It is really tricky, but I believe there should be a path for an optimization. I am having some difficulty navigating the code to find it though, so it might take me a few attempt and some more time building up the confidence. Sorry for being to quick on the first try. You are right in that it is incorrect, the patch can give two distinct Swift extensions the same key which it sounds like you also conclude, and I did mean to affect that logic to bypass the needed cases. I will however prefer to close this, if you know the rework will make it obsolete. It sounds like it is coming soon, so this might not be worth the risk, if I am not more confident in this codebase :-) EDIT: Closing it. Even though profiling suggests there is a performance opportunity here generating imported-extension identifiers currently materializes unrelated SDK declarations. I believe that work could be reduced, but I am not familiar enough in this codebase to find a clean implementation that preserves identifier uniqueness in every case. The current patch does not meet that requirement. I am adding this info to ensure the reasoning for closing it on the PR, in the hopes that it might help somehow or somewhere :-) Thanks for the help @jketema! |
I can always try to review more, but as I wrote on the issue. This is something we would like to spend as little time on as possible, as we're working on a replacement solution. |
What?
Avoid SDK-wide declaration indexing when naming imported Swift extensions by reusing native Clang identities. Internal imported-extension TRAP keys change; extraction scope stays unchanged.
Why?
Generating these identifiers currently materializes unrelated SDK declarations, adding unnecessary work and slowing Swift CodeQL extraction.
Fixes #22771
AI assistance: GitHub Copilot assisted with implementation and validation.