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

[Experiment] Export generic instances from libstd. #68673

Conversation

michaelwoerister
Copy link
Member

This should resolve issue #64140. However it is unclear if there are detrimental effects. Let's test if there are performance improvements to be had.

r? @ghost

@michaelwoerister
Copy link
Member Author

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Collaborator

bors commented Jan 30, 2020

⌛ Trying commit 73e77a7 with merge f8a2991...

bors added a commit that referenced this pull request Jan 30, 2020
…try>

[Experiment] Export generic instances from libstd.

This should resolve issue #64140. However it is unclear if there are detrimental effects. Let's test if there are performance improvements to be had.

r? @ghost
@bors
Copy link
Collaborator

bors commented Jan 30, 2020

☀️ Try build successful - checks-azure
Build commit: f8a2991 (f8a2991ec6f9a64de373fd9ee5900bb0da828d42)

@rust-timer
Copy link
Collaborator

Queued f8a2991 with parent 212b2c7, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit f8a2991, comparison URL.

@bjorn3
Copy link
Member

bjorn3 commented Jan 31, 2020

This seems to negatively affect at least linker performance.

@panstromek
Copy link
Contributor

panstromek commented Feb 1, 2020

Without actually understanding anything - it's interesting that all most regressed benchmarks are artificial little programs and stress tests, while the ones that improved are more "real" programs, like cargo, style-servo, ripgrep or inflate. Even though the page is full of red numbers, it's bit misleading because they all come from 50 LOC programs. Is that enough to even derive some conclusion?

[edit] - not entirely true, nvm

@bjorn3
Copy link
Member

bjorn3 commented Feb 1, 2020

ripgrep-opt clean incremental regresses by 1.9% (linker and some other things). cranelift-codegen-opt baseline incremental regresses by 1.2% (mostly LLVM). Both are "real" programs/libraries.

@michaelwoerister
Copy link
Member Author

Yes, it looks like this change causes quite a bit of overhead for loading the list of generic symbols available in the standard library. Small programs that then do not use any of these newly available symbols don't get a chance for amortizing that overhead.

ripgrep-opt clean incremental regresses by 1.9% (linker and some other things). cranelift-codegen-opt baseline incremental regresses by 1.2% (mostly LLVM). Both are "real" programs/libraries.

I wouldn't rely too much on wall-time numbers when it comes to changes below 3%. They are rather noisy.

I'm not quite sure how to proceed here. Those performance numbers clearly aren't a mandate to merge this :)

@bjorn3
Copy link
Member

bjorn3 commented Feb 3, 2020

I was looking at the instructions count.

@hdhoang hdhoang added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 6, 2020
@JohnCSimon JohnCSimon added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants