memoize the common fragments calculation for diffing overloads #992
Conversation
QuietMisdreavus
requested review from
patshaughnessy,
d-ronnqvist,
daniel-grumberg and
mayaepps
|
@swift-ci Please test |
|
@swift-ci Please test Linux |
| /// Translates a symbol's declaration into a render node's Declarations section. | ||
| struct DeclarationsSectionTranslator: RenderSectionTranslator { | ||
| /// A mapping from a symbol reference to the "common fragments" in its overload group. | ||
| static let commonFragmentsMap: Synchronized<[ResolvedTopicReference: [SymbolGraph.Symbol.DeclarationFragments.Fragment]]> = .init([:]) |
There was a problem hiding this comment.
Why define this as static? It will work fine, I think, but would fail someday if we ever compile more than one documentation project in the same DocC process, because we would have values left over from previous runs. As written, we assume the map will only ever be used once. Might be safer to just make this a normal attribute of DeclarationsSectionTranslator.
There was a problem hiding this comment.
DeclarationsSectionTranslator instances are not shared between different symbols' renderings. If i made this an instance property, the saved results would not be available the next time around.
There was a problem hiding this comment.
I see, ok makes sense.
| fragments: [SymbolGraph.Symbol.DeclarationFragments.Fragment]) | ||
| { | ||
| Self.commonFragmentsMap.sync({ map in | ||
| for reference in references { |
There was a problem hiding this comment.
Why bother saving the fragments for all of the overload declarations? Reading this, I'm surprised that setCommonFragments takes an array of references, but commonFragments only queries one.
There was a problem hiding this comment.
The entire trick being relied upon here is that the list of common fragments is going to be the same for a group of overloaded declarations. So the first one in a group that gets rendered can save the calculation for the whole group. So when we check for a cached result, we only need to check for one of the declarations.
There was a problem hiding this comment.
So the mainDeclaration will change depending on when we call commonFragments below? Thanks for the explanation.
| for mainDeclaration: OverloadDeclaration, | ||
| overloadDeclarations: [OverloadDeclaration] | ||
| ) -> [SymbolGraph.Symbol.DeclarationFragments.Fragment] { | ||
| if let fragments = commonFragments(for: mainDeclaration.reference) { |
There was a problem hiding this comment.
See the previous comment. If you only ever query the fragments for the main declaration here, then why save the same fragments for each of the overloads in the call to setCommonFragments below?
|
|
||
| // Collect the "common fragments" so we can highlight the ones that are different | ||
| // in each declaration | ||
| let commonFragments = longestCommonSubsequence(preProcessedDeclarations) | ||
| let commonFragments = commonFragments( |
There was a problem hiding this comment.
Again, it appears you only ever need the common fragments for the main declaration, and not for the individual overloads.
|
@swift-ci Please test |
Bug/issue #, if applicable: None
Summary
This PR refactors the overload declaration diffing code to memoize the results so that they don't need to be recalculated. The original diffing code recalculates the "common fragments" every time it needs to render a page. However, this wastes work for overloads in the same group, since each symbol's page needs to be calculated individually. By saving those results, we can load the common fragments without having to perform an expensive calculation.
This benchmark was run on a large framework with many overloaded methods:
Dependencies
None
Testing
Ensure that no functionality has changed.
Checklist
Make sure you check off the following items. If they cannot be completed, provide a reason.
./bin/testscript and it succeeded