Skip to content

memoize the common fragments calculation for diffing overloads #992

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Aug 6, 2024
Merged

memoize the common fragments calculation for diffing overloads #992

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
52ee462
memoize the common fragments calculation
QuietMisdreavus Jul 22, 2024
06fe112
Merge branch 'main' into memoize-diff
QuietMisdreavus Aug 5, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 52 additions & 7 deletions ...ces/SwiftDocC/Model/Rendering/RenderSectionTranslator/DeclarationsSectionTranslator.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,57 @@
import Foundation
import SymbolKit

typealias OverloadDeclaration = (
declaration: [SymbolGraph.Symbol.DeclarationFragments.Fragment],
reference: ResolvedTopicReference
)

/// 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([:])
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, ok makes sense.


/// Set the common fragments for the given symbol references.
func setCommonFragments(
references: [ResolvedTopicReference],
fragments: [SymbolGraph.Symbol.DeclarationFragments.Fragment])
{
Self.commonFragmentsMap.sync({ map in
for reference in references {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So the mainDeclaration will change depending on when we call commonFragments below? Thanks for the explanation.

map[reference] = fragments
}
})
}

/// Fetch the common fragments for the given reference, if present.
func commonFragments(
for reference: ResolvedTopicReference
) -> [SymbolGraph.Symbol.DeclarationFragments.Fragment]? {
Self.commonFragmentsMap.sync({ $0[reference] })
}

/// Fetch the common fragments for the given references, or compute it if necessary.
func commonFragments(
for mainDeclaration: OverloadDeclaration,
overloadDeclarations: [OverloadDeclaration]
) -> [SymbolGraph.Symbol.DeclarationFragments.Fragment] {
if let fragments = commonFragments(for: mainDeclaration.reference) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

return fragments
}

let preProcessedDeclarations = [mainDeclaration.declaration] + overloadDeclarations.map(\.declaration)

// Collect the "common fragments" so we can highlight the ones that are different
// in each declaration
let commonFragments = longestCommonSubsequence(preProcessedDeclarations)

setCommonFragments(
references: [mainDeclaration.reference] + overloadDeclarations.map(\.reference),
fragments: commonFragments)

return commonFragments
}

func translateSection(
for symbol: Symbol,
renderNode: inout RenderNode,
Expand Down Expand Up @@ -79,11 +128,6 @@ struct DeclarationsSectionTranslator: RenderSectionTranslator {
return postProcessTokens(translatedDeclaration)
}

typealias OverloadDeclaration = (
declaration: [SymbolGraph.Symbol.DeclarationFragments.Fragment],
reference: ResolvedTopicReference
)

func renderOtherDeclarationsTokens(
from overloadDeclarations: [OverloadDeclaration],
displayIndex: Int,
Expand Down Expand Up @@ -162,11 +206,12 @@ struct DeclarationsSectionTranslator: RenderSectionTranslator {
let processedOverloadDeclarations = overloadDeclarations.map({
OverloadDeclaration($0.declaration.flatMap(preProcessFragment(_:)), $0.reference)
})
let preProcessedDeclarations = [mainDeclaration] + processedOverloadDeclarations.map(\.declaration)

// Collect the "common fragments" so we can highlight the ones that are different
// in each declaration
let commonFragments = longestCommonSubsequence(preProcessedDeclarations)
let commonFragments = commonFragments(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, it appears you only ever need the common fragments for the main declaration, and not for the individual overloads.

for: (mainDeclaration, renderNode.identifier),
overloadDeclarations: processedOverloadDeclarations)

renderedTokens = translateDeclaration(
mainDeclaration,
Expand Down