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.

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

Recover performance loss after PagedVector introduction #67972

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

Recover performance loss after PagedVector introduction #67972

wants to merge 4 commits into from

Conversation

ktf
Copy link
Contributor

@ktf ktf commented Oct 2, 2023

No description provided.

@ktf ktf mentioned this pull request Oct 2, 2023
Merged
@vgvassilev vgvassilev requested a review from nikic October 2, 2023 11:20
@ktf
Copy link
Contributor Author

ktf commented Oct 2, 2023

@nikic any suggestion on how can I run the performance benchmark myself?

@nikic
Copy link
Contributor

nikic commented Oct 2, 2023

Not seeing a difference in instructions:u with this change: http://llvm-compile-time-tracker.com/compare.php?from=3b25407d977d9ed3dbcaccd4f8850b65e95d70fd&to=8f5c0fd7348e78aedafcee3f7e67f04e3bc79e9c&stat=instructions:u Which is not really surprising -- it might make a difference in cycles, but not at a level I can resolve accurately.

@ktf
Change PageSize to 32 elements
f119cca 
Moves from 42 elements (automatically calculated) to 32. It should
both reduce memory usage (it does at least in my test) and avoid
the need for a division when indexing the elements.
@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Oct 2, 2023
@ktf ktf changed the title Hint for branch likelihood Recover performance loss after PagedVector introduction Oct 2, 2023
@ktf
Copy link
Contributor Author

ktf commented Oct 2, 2023

@nikic I have pushed a new commit where I change the size of the page to 32 elements from the current 42. It should at least get rid of a division.

@cor3ntin
Copy link
Contributor

Should we abandon this?

@ktf
Copy link
Contributor Author

ktf commented May 29, 2024

I have updated the branch and I think the two changes in this PR are in any case good. However it's my understanding that the performance regression was actually due to some less aggressive inlining of the new code. I have no idea how to proceed for that though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nikic nikic Awaiting requested review from nikic

Assignees
No one assigned
Labels
clang:frontend Language frontend issues, e.g. anything involving "Sema" clang Clang issues not falling into any other category llvm:adt
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@ktf @nikic @cor3ntin @llvmbot