-
Notifications
You must be signed in to change notification settings - Fork 167
[Low level] Store SourceLanguage properties outside of the main structure use a new bit-set type for "sets" of languages #1355
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.
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
base: main
Are you sure you want to change the base?
Conversation
…separately This idea came from after realizing that in practice accesses are _almost_ exclusively `id`, `==`, or `<`.
|
@swift-ci please test |
|
@swift-ci please test |
| } | ||
|
|
||
| @Test() | ||
| func testCombinations() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI: This is a moved test (and implementation) from before.
|
|
||
| struct FixedSizeBitSetTests { | ||
| @Test | ||
| func testBehavesSameAsSet() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI: This is a moved test (and implementation) from before.
|
@swift-ci please test |
patshaughnessy
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Amazing work! Just some naming suggestions and a few questions...
| private static func _accessInfo(id: UInt8) -> _SourceLanguageInformation { | ||
| let (unknownIndex, isKnownLanguage) = id.subtractingReportingOverflow(SourceLanguage._numberOfKnownLanguages) | ||
| return if isKnownLanguage { | ||
| _knownLanguages[Int(id)] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why does Swift require a cast here? We can't index an array using UInt8?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct. The Index type for Array is plain Int so we have to cast the id value.
| .map { $0.lowercased() } | ||
| .contains(id) | ||
| private static func knownLanguage(withName name: String) -> SourceLanguage? { | ||
| switch name.lowercased() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be ideal not to have to repeat all of the language names like this. Is there a way to refactor this to iterate over the _knownLanguages array somehow?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, this could be implemented as an iteration over _knownLanguages like this:
let name = name.lowercased()
let index = _knownLanguages.firstIndex(where: {
$0.name == name
})
return index.map { SourceLanguage_new(_id: UInt8($0)) }Very similarly, _knownLanguage(withIdentifier:) below could be implemented as an iteration like this:
let id = id.lowercased()
let index = _knownLanguages.firstIndex(where: {
$0.id == id || $0.idAliases.contains(id)
})
return index.map { SourceLanguage_new(_id: UInt8($0)) }I had speculated that the switch implementations would be faster—thinking that the compiler would have more information to go on to optimize the code—but I didn't actually try and measure anything until now.
With the switch cases in the current code, the Swift compiler—when compiling with optimizations (a release build)—creates assembly that corresponds to a series of if checks one after another whereas with the firstIndex(where:) implementation, it creates assembly that corresponds to a basic loop. Essentially you can think of the difference as being between a loop that's been unrolled and one that hasn't been.
In micro benchmarks, the switch implementation for knownLanguage(withName:) is ~2.5× faster than the iteration implementation for known values (e.g. "Swift") and ~2× faster for unknown values (e.g. "Banana"). For _knownLanguage(withIdentifier:) the switch implementation is about ~4.5× faster for known values and ~4 × times faster for unknown values.
Because both initializers are being called quite frequently (>10 times per page), these differences could add up.
Also, because the list of known languages is unlikely to change frequently (possibly not for years), I find that the little bit of code duplication within this file is worth it for these initializers.
|
|
||
| // MARK: SourceLanguage Set | ||
|
|
||
| package struct SmallSourceLanguageSet: Sendable, Hashable, SetAlgebra, ExpressibleByArrayLiteral, Sequence, Collection { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe use a separate Swift file for SmallSourceLanguageSet ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It needs to be defined in the same file in order to be able to access _id and init(_id:) which have fileprivate access. The alternative would be to increase the to internal access so that (all) other files in this module can access them.
...ces/SwiftDocC/Infrastructure/Link Resolution/PathHierarchy+TypeSignatureDisambiguation.swift
Show resolved
Hide resolved
| // MARK: SourceLanguage Set | ||
|
|
||
| package struct SmallSourceLanguageSet: Sendable, Hashable, SetAlgebra, ExpressibleByArrayLiteral, Sequence, Collection { | ||
| // There are a few different valid ways that we could implement this, each with their own tradeoffs. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And could you just name this SourceLanguageSet ? Do we need "Small" in the name?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My intention was to have some kind of indication that it's has a size limitation, unlike a regular Set<SourceLanguage> would. I thought of prefixing it with "FixedWidth" but "Small" felt less technical.
| self.init(bundleID: bundleID, path: path, fragment: fragment, _smallSourceLanguages: .init(sourceLanguages)) | ||
| } | ||
|
|
||
| init(bundleID: DocumentationBundle.Identifier, path: String, fragment: String? = nil, _smallSourceLanguages: SmallSourceLanguageSet) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you just name this sourceLanguages ? It's a bit odd reading "small" at every call site, and also why the underscore?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can rename the parameter for the initializer because overloads can be distinguished by their parameter types but the property needs to be named something other than sourceLanguages because that's what the public Set<SourceLanguage> property is already called and renaming that or changing its type would be an API breaking change.
| XCTAssertEqual(SourceLanguage(knownLanguageIdentifier: "objc"), .objectiveC) | ||
| XCTAssertEqual(SourceLanguage(knownLanguageIdentifier: "c"), .objectiveC) | ||
| struct SourceLanguageTests { | ||
| @Test(arguments: SourceLanguage.knownLanguages) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice use of parameterized tests!
|
@swift-ci please test |
Bug/issue #, if applicable:
Summary
This is a series of low-level optimizations regarding the use of
SourceLanguageand particularlySet<SourceLanguage>.Note
These performance improvements (see below) adds a limitation that a single
docc convertcall can't define more than 64 differentSourceLanguagevalues. However, considering that C/C++/Objective-C is rolled into one and that anything more than 2 languages in the same project is very rare—and anything above 3 languages being practically unheard of—I don't think this limitations is going to be impactful in practice.These ideas came from a realization that DocC uses
SourceLanguagealmost exclusively for equality checks in one of 3 forms:==(_:_:)comparisons between full valuesidpropertiesSet<SourceLanguage>(which doeshash(into:) and==(::)` behind the scenes.I confirmed this hypothesis by adding a local counter whenever a
SourceLanguagevalues was created, checked for equality, checked for comparison, hashed, and whenever any of its properties were accessed. This showed that across a full build, DocC does on average ~50==(_:_:)calls per page, on average ~140hash(into:)calls per page, and on average ~100idaccesses page page.Based on these numbers I hypothesized that it would be worthwhile to optimize
SourceLanguagefor quick comparisons at the cost of slower accesses ofnameand other properties.At first I though about only storing the
idstring in the structure and accessing the other properties through indirect storage but then I thought that—because the most common use ofSourceLanguagevalues is to put them in aSetand because in practice, projects are expected to have very few different languages (low single digits)—if the identifier was numeric, sets of languages could be represented as bit set. Using a private numeric ID would mean that the simplersomeLanguage == .swiftwould be faster thansomeLanguage.id == "swift"which a lot of existing code was doing.I reimplemented the internal of
SourceLanguagein 10dabb4 and 4fdb68c. Then in f27559e, 4caf109, 090cad1, 3e1396a, and b9a6030 I generalized and improved the exist fixed-width bit set type—that DocC uses for type signature disambiguation—to finally be able to add a bit-set backed type that represents a "set" ofsource languages in 6ac19ea.
After that; 5273e94, d6e63ae, and 3d3d580 each updated other existing code in DocC to favor full
SourceLanguagecomparisons and favor the bit-set backed "set" type in internal implementation details.Trading
idaccesses for==(_:_:)checks like this, increased the number of==(_:_:)checks by ~3× (on average ~150 calls per page) but reduced the number ofidaccesses by~3× (on average ~30 calls per page)~4.5× (on average ~20 calls per page) after 9c8a9c1. The remainingidcalls is largely caused by the RenderJSON code which uses string identifiers in an enum that can't would require source breaking changes to update. Because existing API need to surfaceSet<SourceLanguage>API externally, while using a bit-set internally, the number ofSourceLanguage.hash(into:)calls increased by ~1.3×, but the newhash(into:)implementation is ~10× faster, so that's still a net-positive.Additionally, the changes to use the new bit-set backed type for "sets" of languages resulted in a large number of method calls moving from
Set<SourceLanguage>to the newSmallSourceLanguageSet. For example:inset(_:)calls per page, which is ~15× faster in micro benchmarkscontains(_:)calls per page, which is ~100× faster in micro benchmarksintersection(_:)calls per page, which is >1000× faster in micro benchmarksmin()calls per page, which is ~150× faster in micro benchmarksIn aggregate, on the scale of an entire documentation build, these add up to a small but measurable improvement. In one large (~10k page) Swift-only framework I measured these time and memory improvements (on my machine):
In another large (~10k pages) framework with both Swift and Objective-C project symbols I measured similar (~2%) improvements.
Dependencies
None.
Testing
Nothing in particular. This isn't a user-facing change.
Checklist
Make sure you check off the following items. If they cannot be completed, provide a reason.
./bin/testscript and it succeeded[ ] Updated documentation if necessary