Skip to content
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

[DNM] [stdlib] Performance improvements for reading keypaths #70451

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

Azoy
Copy link
Member

@Azoy Azoy commented Dec 13, 2023

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
Member Author

Azoy commented Dec 13, 2023

@swift-ci please smoke test

@Azoy
Copy link
Member Author

Azoy commented Dec 13, 2023

@swift-ci please benchmark

@Azoy
Copy link
Member Author

Azoy commented Dec 13, 2023

@swift-ci please benchmark Apple Silicon

@Azoy
Copy link
Member Author

Azoy commented Dec 14, 2023

@swift-ci please smoke test

@Azoy
Copy link
Member Author

Azoy commented Dec 14, 2023

@swift-ci please benchmark Apple Silicon

@Azoy
Copy link
Member Author

Azoy commented Dec 14, 2023

@swift-ci please benchmark

@Azoy
Copy link
Member Author

Azoy commented Dec 16, 2023

@swift-ci please smoke test

@Azoy
Copy link
Member Author

Azoy commented Dec 16, 2023

@swift-ci please benchmark

@Azoy
Copy link
Member 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
Member

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
Member

@swift-ci build toolchain

@Azoy
Copy link
Member 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.

@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
Member 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants