Skip to content

[stdlib] Performance improvements for reading keypaths #70451

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

Jump to bottom
Merged
merged 10 commits into from
Feb 6, 2025
Merged

[stdlib] Performance improvements for reading keypaths #70451

merged 10 commits into from
Feb 6, 2025

Conversation

Azoy
Copy link
Contributor

@Azoy Azoy commented Dec 13, 2023 •
edited
Loading

This patch includes some performance improvements for reading out a value from a key path. Essentially I've cut out all metadata allocations within the projection loop as well as getting rid of the intermediate Any value we were using to traffic intermediate values. Instead, we calculate the largest type we'll ever store during instantiation and stuff that value at the end of the buffer. We'll read this value when we project and create a temporary allocation and constantly re-view this memory for components.

Partially resolves: rdar://116587442

@Azoy Azoy requested a review from jckarter December 13, 2023 22:13
@Azoy
Copy link
Contributor Author

Azoy commented Dec 13, 2023

@swift-ci please smoke test

@Azoy
Copy link
Contributor Author

Azoy commented Dec 13, 2023

@swift-ci please benchmark

@Azoy
Copy link
Contributor Author

Azoy commented Dec 13, 2023

@swift-ci please benchmark Apple Silicon

@Azoy
Copy link
Contributor Author

Azoy commented Dec 14, 2023

@swift-ci please smoke test

@Azoy
Copy link
Contributor Author

Azoy commented Dec 14, 2023

@swift-ci please benchmark Apple Silicon

@Azoy
Copy link
Contributor Author

Azoy commented Dec 14, 2023

@swift-ci please benchmark

@Azoy
Copy link
Contributor Author

Azoy commented Dec 16, 2023

@swift-ci please smoke test

@Azoy
Copy link
Contributor Author

Azoy commented Dec 16, 2023

@swift-ci please benchmark

@Azoy
Copy link
Contributor Author

Azoy commented Dec 18, 2023

@swift-ci please test

@stephencelis
Copy link
Contributor

@Azoy I actually just encountered a crash in _projectReadOnly: #70611

Curious if this branch's refactor fixes?

@phausler
Copy link
Contributor

phausler commented Jan 2, 2024

@swift-ci please build macOS toolchain

@stephencelis
Copy link
Contributor

@phausler Should it be "please build toolchain macOS"? Or just "please build toolchain"? I'm not seeing anything in the CI queue.

@stephencelis
Copy link
Contributor

@swift-ci please build toolchain

@MaxDesiatov
Copy link
Contributor

@swift-ci build toolchain

@Azoy
Copy link
Contributor Author

Azoy commented Jan 5, 2024

@stephencelis sorry for the late reply, I tried out the code snippet you linked with this change and it still crashes at runtime. I think this is the compiler doing something incorrectly perhaps, but maybe it's in the keypath appending logic. I'll take a closer look.

@stephencelis stephencelis mentioned this pull request Feb 2, 2024
Open
@Azoy Azoy force-pushed the read-keypath-optimization branch from e381555 to 3b0a35d Compare February 20, 2024 19:01
@Azoy Azoy marked this pull request as ready for review February 20, 2024 19:02
@Azoy Azoy requested a review from a team as a code owner February 20, 2024 19:02
@Azoy
Copy link
Contributor Author

Azoy commented Feb 20, 2024

@swift-ci please test

@Azoy Azoy force-pushed the read-keypath-optimization branch from 3b0a35d to 1ca08c1 Compare May 7, 2024 16:12
@mbrandonw mbrandonw mentioned this pull request Jul 13, 2024
Open
@drexin
Copy link
Contributor

drexin commented Oct 28, 2024

@swift-ci please test

@Azoy Azoy force-pushed the read-keypath-optimization branch from 1ca08c1 to 1f949a5 Compare October 31, 2024 17:30
@Azoy Azoy requested a review from a team as a code owner October 31, 2024 17:30
@Azoy Azoy changed the title [DNM] [stdlib] Performance improvements for reading keypaths [stdlib] Performance improvements for reading keypaths Oct 31, 2024
@Azoy
Copy link
Contributor Author

Azoy commented Oct 31, 2024

@swift-ci please test

@Azoy
Copy link
Contributor Author

Azoy commented Oct 31, 2024

@swift-ci please benchmark Apple silicon

@Azoy
Copy link
Contributor Author

Azoy commented Nov 5, 2024

@swift-ci please test

@Azoy
Copy link
Contributor Author

Azoy commented Nov 5, 2024

@swift-ci please benchmark

@Azoy
Copy link
Contributor Author

Azoy commented Jan 7, 2025

@swift-ci please test

@Azoy
Copy link
Contributor Author

Azoy commented Jan 7, 2025

@swift-ci please benchmark

@natecook1000
Copy link
Member

@swift-ci Please Build Toolchain macOS Platform

@Azoy Azoy force-pushed the read-keypath-optimization branch from c0571a5 to 90bd2a0 Compare February 5, 2025 00:21
@Azoy
Copy link
Contributor Author

Azoy commented Feb 5, 2025

@swift-ci please test

@Azoy
Copy link
Contributor Author

Azoy commented Feb 5, 2025

@swift-ci please benchmark

@Azoy
Copy link
Contributor Author

Azoy commented Feb 5, 2025

@swift-ci please test

@Azoy
Copy link
Contributor Author

Azoy commented Feb 5, 2025

@swift-ci please benchmark

@Azoy
Copy link
Contributor Author

Azoy commented Feb 6, 2025

Performance (x86_64): -O

Regression OLD NEW DELTA RATIO
Dict.FilterAllMatch.24k 33.739 72.647 +115.3% 0.46x
Dict.FilterAllMatch.20k 28.539 61.175 +114.4% 0.47x
Dict.FilterAllMatch.16k 23.375 49.896 +113.5% 0.47x
Dict.FilterAllMatch.28k 41.5 88.357 +112.9% 0.47x
Set.filter.Int100.24k 31.063 62.225 +100.3% 0.50x
Set.filter.Int100.20k 26.283 52.447 +99.5% 0.50x
Set.filter.Int100.16k 21.56 42.769 +98.4% 0.50x
Set.filter.Int100.28k 38.271 75.781 +98.0% 0.51x
ArrayAppendGenericStructs 1322.5 1786.667 +35.1% 0.74x (?)
StringDistance.utf16.ascii 112.143 151.0 +34.6% 0.74x
Set.subtracting.Seq.Box.Empty 107.818 130.722 +21.2% 0.82x (?)
Set.isSubset.Int.Empty 45.72 53.889 +17.9% 0.85x (?)
Set.isDisjoint.Int.Empty 46.833 55.0 +17.4% 0.85x (?)
Set.isSuperset.Seq.Empty.Int 44.654 51.071 +14.4% 0.87x (?)
CharacterLiteralsLarge 63.182 71.87 +13.8% 0.88x (?)
StringFromLongWholeSubstring 2.719 3.042 +11.9% 0.89x (?)
DataCreateEmptyArray 762.651 852.941 +11.8% 0.89x (?)
Set.filter.Int50.28k 295.143 329.333 +11.6% 0.90x (?)
ArrayInClass 195.962 217.727 +11.1% 0.90x (?)
ArraySetElement 294.0 326.5 +11.1% 0.90x (?)
Array2D 4384.0 4824.0 +10.0% 0.91x (?)
StringAdder 290.5 319.143 +9.9% 0.91x (?)
StringBuilder 287.0 311.5 +8.5% 0.92x (?)
 
Improvement OLD NEW DELTA RATIO
KeyPathOptionals 657.0 118.579 -82.0% 5.54x
KeyPathMutatingGetset 216.5 42.431 -80.4% 5.10x
KeyPathGet 197.0 40.655 -79.4% 4.85x
KeyPathClassStructs 466.5 113.583 -75.7% 4.11x
KeyPathsSmallStruct 20.889 5.59 -73.2% 3.74x
KeyPathNestedClasses 187.182 55.558 -70.3% 3.37x
KeyPathReadPerformance 25.044 9.781 -60.9% 2.56x
KeyPathNestedStructs 8.345 3.462 -58.5% 2.41x
UTF8Decode_InitFromData 170.636 134.846 -21.0% 1.27x (?)
UTF8Decode_InitFromBytes 169.9 138.455 -18.5% 1.23x (?)
DictionaryOfAnyHashableStrings_insert 3756.667 3330.0 -11.4% 1.13x (?)
NormalizedIterator_nonBMPSlowestPrenormal 489.0 439.63 -10.1% 1.11x (?)
DictionaryOfAnyHashableStrings_lookup 2401.0 2181.333 -9.1% 1.10x (?)
NormalizedIterator_emoji 373.92 343.704 -8.1% 1.09x (?)
Dictionary4OfObjects 243.2 224.167 -7.8% 1.08x (?)

@Azoy Azoy merged commit 57234f8 into swiftlang:main Feb 6, 2025
6 checks passed
@Azoy Azoy deleted the read-keypath-optimization branch February 6, 2025 06:57
jckarter
jckarter reviewed Feb 6, 2025
View reviewed changes
stdlib/public/core/KeyPath.swift
if _fastPath(tag == 0) {
// Optional "shares" a layout with its Wrapped type meaning we can
// reinterpret the base address as an address to its Wrapped value.
pointer.initialize(to: Builtin.reinterpretCast(base))
Copy link
Contributor

Choose a reason for hiding this comment

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

Where do we ensure the tag byte, if any, after the base value is zero?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure I understand what you mean here 🤔

Copy link
Contributor

@jckarter jckarter Feb 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

If Optional<T> is larger than T (because T doesn't have any extra inhabitants) then the tag byte after the payload needs to be zero to indicate the some case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh I check that right before this:

      // Optional's tags are some = 0, none = 1
       let tag = UInt32(Builtin.getEnumTag(base))

I call the vwt to get the enum tag of the value and if it's 0 then I know I can bit cast the optional value to its wrapped one.

@jmschonfeld jmschonfeld mentioned this pull request Feb 14, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jckarter jckarter jckarter left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@Azoy @stephencelis @phausler @MaxDesiatov @drexin @natecook1000 @jckarter