[SymbolGraph] Make symbol-graph output deterministic #80648
Conversation
cachemeifyoucan
requested review from
artemcm,
tshortli and
QuietMisdreavus
as code owners
|
@swift-ci please smoke test |
lib/SymbolGraphGen/SymbolGraph.cpp
Outdated
| SmallString<256> LHSTargetUSR, RHSTargetUSR; | ||
| LHS.Target.getUSR(LHSTargetUSR); | ||
| RHS.Target.getUSR(RHSTargetUSR); | ||
| return LHSTargetUSR < RHSTargetUSR; |
There was a problem hiding this comment.
I know this is code you didn't touch, but i feel like this should order on more than target USR - there can be several relationships that target the same symbol which are still going to be in a nondeterminstic ordering after this sort is applied.
Personally i would define a < operator for Edge which compares target, source, and kind, and defer to that in this sort method.
There was a problem hiding this comment.
Sounds good. Is USR good enough for Symbol entry?
There was a problem hiding this comment.
Technically no. Imported Objective-C symbols that have an automatically-converted version (e.g. a completion handler being converted into an async method) will appear in symbol graphs as two symbols with the same USR. I'm not sure how to fetch that info offhand, but being able to match those would be useful as well.
In SymbolKit we would just defer to the declaration fragments, but we don't store those outside the output JSON structure, so i'm not sure what could be used as an ordering constraint in this case.
There was a problem hiding this comment.
Maybe it is easier if just convert DenseSet to llvm::SetVector? It uses more memory but assuming the ASTWalker is deterministic, then preserving the order of insertion should make symbol graph deterministic too.
There was a problem hiding this comment.
I'm not sure if ASTWalker (SourceEntityWalker in our case) is deterministic, but if it is then that should be perfect.
There was a problem hiding this comment.
A regular ASTWalker should be, unless some function overlayed on top is breaking this, then it is a different problem to fix. I run it in a loop for a while and so far nothing is non-deterministic. Let me update to use that.
|
Thanks so much for opening this PR! This is something that's been in my awareness for a long time. I've previously filed this issue as #59602 and rdar://95598551, for reference. Can you update the relationships sorting method like i commented? |
Ah, nice. I am not aware of that. Let me update to use your issue and radar instead. |
SymbolGraph generation iterating over llvm::DenseSet, which makes the symbols and relationship fields appear in non-deterministic ordering. Switch to use llvm::SetVector to preserve the insertion order from SourceEntityWalker to make order deterministic. Resolves: swiftlang#59602
cachemeifyoucan
force-pushed
the
eng/PR-148839911
branch
from
f45e2fb to
dfa4a27
Compare
|
@swift-ci please smoke test |
|
@ahoppen Since I am removing your sorting function, does the order of edges matter to you or it just need to be deterministic? |
|
The only need for the sorting was that we get consistent output to compare symbol graphs. If we get deterministic output by other means now, the sorting shouldn’t be needed anymore. |
SymbolGraph generation iterating over llvm::DenseSet, which makes the symbols and relationship fields appear in non-deterministic ordering. Switch to use llvm::SetVector to preserve the insertion order from SourceEntityWalker to make order deterministic.
Resolves: #59602