Much faster navigator index creation for mixed Swift and Objective-C projects #917
Conversation
|
@swift-ci please test |
|
Oh, the CI fails because we're not allowed to nest protocols like that yet. I'll move them out to be top-level tomorrow. |
|
@swift-ci please test |
| import Foundation | ||
|
|
||
| /// A language specific representation of a render node value for building a navigator index. | ||
| protocol NavigatorIndexableRenderNodeRepresentation<Metadata> { |
There was a problem hiding this comment.
It seems like this protocol and NavigatorIndexableRenderMetadataRepresentation only are conformed to once. Why do we need this protocol as opposed to just using RenderNodeVariantView directly, for example?
There was a problem hiding this comment.
Both protocols are conformed to twice; once for the main type and once for the view:
Render Node representation
extension RenderNode: NavigatorIndexableRenderNodeRepresentation {}struct RenderNodeVariantView: NavigatorIndexableRenderNodeRepresentation {
...
}Metadata representation
extension RenderMetadata: NavigatorIndexableRenderMetadataRepresentation {}struct RenderMetadataVariantView: NavigatorIndexableRenderMetadataRepresentation {
...
}We need these protocols so that the same index implementation can be used for both Swift and Objective-C representations of pages.
private func index(_ renderNode: any NavigatorIndexableRenderNodeRepresentation, traits: [RenderNode.Variant.Trait]?) throws -> InterfaceLanguage? { ... }There was a problem hiding this comment.
Got it, I'd missed that. Thanks for the explanation!
| } | ||
| } | ||
|
|
||
| private let typesThatShouldNotUseNavigatorTitle: Set<NavigatorIndex.PageType> = [ |
There was a problem hiding this comment.
Why do we want these types to not use the navigator title? I assume this is not new behavior--is it because they're symbols?
There was a problem hiding this comment.
I don't know. This isn't new code. I just moved it from here because I needed to make the code usable for NavigatorIndexableRenderNodeRepresentation and not just RenderNode.
| RenderRelationshipsGroup( | ||
| name: chapter.name, | ||
| abstract: nil, | ||
| references: chapter.tutorials.compactMap { self.references[$0.identifier] as? TopicRenderReference } |
There was a problem hiding this comment.
Are there any scenarios where the volume's chapters wouldn't be TopicRenderReferences? It looks like previously this logic force-unwrapped the result of the cast.
There was a problem hiding this comment.
I'm not aware of any scenarios today but maybe there will be some in the future. I saw this code and felt that it was unnecessary to force-cast when the old code was already doing a compactMap to access the references anyway:
+ tutorials.compactMap { self.references[$0.identifier] as? TopicRenderReference }
- tutorials.compactMap { self.references[$0] } as! [TopicRenderReference]There was a problem hiding this comment.
Ok, makes sense. I was initially thinking it might be useful to have a warning or something here so children weren't silently dropped from the navigator, but it sounds like that's probably not a situation we have to worry about right now.
| /// - originalValue: The original value to transform. | ||
| /// - traits: The traits that determine what variant's patches to apply to the original value. | ||
| /// - Returns: The transformed value, or the original value if no variants match the given traits. | ||
| func applied(to originalValue: Value, for traits: [RenderNode.Variant.Trait]) -> Value { |
There was a problem hiding this comment.
This seems more flexible, nice! Was there something else in this PR that necessitated this change?
There was a problem hiding this comment.
I changed this abstraction because the previous one wasn't all that useful.
There are several properties in the navigator builder that need to determine what the value is for a given trait, some for the render node:
var topicSections: [TaskGroupRenderSection] {
wrapped.topicSectionsVariants.value(for: traits)
}
var defaultImplementationsSections: [TaskGroupRenderSection] {
wrapped.defaultImplementationsSectionsVariants.value(for: traits)
}but most for the render node metadata:
var title: String? {
wrapped.titleVariants.value(for: traits)
}
var navigatorTitle: [DeclarationRenderSection.Token]? {
wrapped.navigatorTitleVariants.value(for: traits)
}
var fragments: [DeclarationRenderSection.Token]? {
wrapped.fragmentsVariants.value(for: traits)
}
var externalID: String? {
wrapped.externalIDVariants.value(for: traits)
}
var roleHeading: String? {
wrapped.roleHeadingVariants.value(for: traits)
}
var symbolKind: String? {
wrapped.symbolKindVariants.value(for: traits)
}
var platforms: [AvailabilityRenderItem]? {
wrapped.platformsVariants.value(for: traits)
}The previous abstraction operated on the individual VariantCollection.Variant (as opposed to operating on the full VariantCollection). This API wasn't particularly useful for two reasons:
- it was the callers responsibility to iterate over the variants for a certain trait
- the API was only available when the value was a (range replaceable) collection
The first drawback meant that to determine the Objective-C value of a render node, each caller needs to access the default value, filter and iterate over the variants, and incrementally apply the patch:
var topicSections: [TaskGroupRenderSection] {
var patchedValue = wrapped.topicSectionsVariants.defaultValue
for variant in wrapped.topicSectionsVariants.variants where variant.traits == traits {
patchedValue = variant.applyingPatchTo(patchedValue)
}
return patchedValue
}Most of this is boilerplate compared to the new API
var topicSections: [TaskGroupRenderSection] {
wrapped.topicSectionsVariants.value(for: traits)
}The second drawback meant that to determine the Objective-C value of render node's metadata's platforms, the caller couldn't use the current API because the Value of VariantCollection<[AvailabilityRenderItem]?> is an optional value and not a collection. Thus, the caller not only needs to access the default value, filter and iterate over the variants, and incrementally apply the patch but also needs to switch over there patch and incrementally apply it depending on the type of Value:
var platforms: [AvailabilityRenderItem]? {
var patchedValue = wrapped.platformsVariants.defaultValue
for variant in wrapped.platformsVariants.variants where variant.traits == traits {
for patch in variant.patch {
switch patch {
case .replace(let value):
patchedValue = value
case .add(nil):
break // Adding nil does nothing
case .add(let value?):
if patchedValue == nil {
patchedValue = value
} else {
patchedValue!.append(contentOf: value)
}
case .remove:
patchedValue = nil
}
}
}
return patchedValue
}The two steps to address all this boilerplate is:
- Create an interface for types that a patch can be applied to
- Move the API to
VariantCollection(instead ofVariantCollection.Value)
The new API did both these things, thus obsoleting the old API.
Further, most of the time the caller doesn't want to apply the patch to an arbitrary value. It makes the most sense to apply the patch to the default value for that variant collection. Because of this, I wrapped the applied(to:for:) API with value(for:).
There was a problem hiding this comment.
That makes sense, thanks for the detailed explanation. The new API is definitely easier to read.
Co-authored-by: Maya Epps <[email protected]>
|
@swift-ci please test |
…projects (swiftlang#917) * Fix a navigator index performance for mixed Swift/Objective-C projects rdar://127759734 * Use new apply-patch function in other places * Deprecate unused `RenderNode/childrenRelationship(for:)` * Move inner function in test to avoid warning about captured state * Fix unrelated issue where the language in disambiguated references wasn't stable * Move new indexable-render-node-representation protocol top-level * Remove extra word in code comment Co-authored-by: Maya Epps <[email protected]> --------- Co-authored-by: Maya Epps <[email protected]>
| guard language == .swift else { | ||
| return | ||
| } |
There was a problem hiding this comment.
Does this means we can't index objective-c only frameworks? is even possible to have an objective-c frameworks or the top level node will always have be a swift node?
There was a problem hiding this comment.
No, any single language framework would already be indexed 3 lines above this
let language = try index(renderNode, traits: nil)| mutating func remove() | ||
| } | ||
|
|
||
| extension Optional: VariantCollectionPatchable where Wrapped: VariantCollectionPatchable { |
|
|
||
| extension Optional: VariantCollectionPatchable where Wrapped: VariantCollectionPatchable { | ||
| mutating func add(_ other: Wrapped?) { | ||
| guard var wrapped, let other else { return } |
There was a problem hiding this comment.
Is wrapped referring to the type of the optional? In this case does it has to be unwrapped?
There was a problem hiding this comment.
wrapped is the optional's value (which is of the type Wrapped)
Bug/issue #, if applicable: rdar://127759734
Summary
This fixes a major inefficiency in the navigator index creation for large (10k+ symbols) mixed Swift and Objective-C projects.
With these changes impacted projects will build about 5 times faster total
docc converttime. Even larger projects may see even faster builds (the biggest improvement that I've seen was ~30 times faster in one project).Benchmarks
Below are two benchmarks from two large mixed Swift and Objective-C projects that show a 6 times improvement and a 5 time improvement compared to current main:
If you look at these closely you may see that the output size and topic graph checksum aren't the same. At first I thought that I caused a regression but it turns out that the output isn't fully stable for mixed Swift and Objective-C projects. I tested this on "main", "release/6.0", and "release/5.10" and they all show similar differences when comparing the same commit of DocC to itself. For example, these 3 measurements are all gathered from the current main (
664acfd7) and show similar changes in output size and topic graph checksum:I've gone over my changes many times and since many previous releases have the same behavior, I'm reasonably certain that I didn't cause them now.
Why this is faster
The added comment in NavigatorIndex.swift explain the specific changes in more detail, but long story short:
Using
RenderNodeVariantOverridesApplier()to get the Objective-C variant of aRenderNodeis very slow and involves repeatedly encoding and decoding the JSON into different formats to transform it.By instead peeking into the different variant collections to give a "view" into the render node's Objective-C data we can avoid all that encoding and decoding.
Unfortunately the
docc process-archive indexcommand still needs to use the slow path. I filed rdar://128050800 to track fixing that as a separate PR.Dependencies
None
Testing
Build documentation for a large mixed Swift and Objective-C project with main and with this branch and measure the total build time. If the project is large enough the difference should be very noticeable. If you want rough estimate numbers to compare you can use
timefor each call, for example:time .build/arm64-apple-macosx/release/docc convert /path/to/SomeCatalog.docc --additional-symbol-graph-dir /path/to/some-symbol-graph-directoryChecklist
Make sure you check off the following items. If they cannot be completed, provide a reason.
./bin/testscript and it succeededUpdated documentation if necessaryNot applicable