-
Notifications
You must be signed in to change notification settings - Fork 13.3k
Also share drop-glue when compiling with -Zshare-generics (i.e. at opt-level=0) #68414
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
Conversation
@bors try @rust-timer queue |
Awaiting bors try build completion |
Also share drop-glue when compiling with -Zshare-generics (i.e. at opt-level=0) This PR adds drop-glue to the set of monomorphizations that can be shared across crates via `-Zshare-generics`. This version of the PR might have detrimental effects on performance as it makes lots of stuff dependent on a single query results (`upstream_monomorphizations_for(def_id_of_drop_in_place)`). That should be fixable but let's do a perf run first. Potentially fixes issue #64140. (cc @alexcrichton) The changes here are related to @matthewjasper's #67332 but should be mostly orthogonal. r? @ghost
☀️ Try build successful - checks-azure |
Queued e9acaef with parent ce361fb, future comparison URL. |
Finished benchmarking try commit e9acaef, comparison URL. |
Hard to get a better try run than that :) r=me on the code here too |
Hm, I find the results a bit underwhelming actually. Let's see if having a separate query drop-glue improves things. If not, merging this simpler version is good too. There's some nice cleanup/documentation in there. |
@bors try @rust-timer queue This adds another query layer for drop-glue so we get better granularity (an approach that could be trivially extend to similar cases like Unfortunately I also found a bug. Running UI tests with |
Awaiting bors try build completion |
Also share drop-glue when compiling with -Zshare-generics (i.e. at opt-level=0) This PR adds drop-glue to the set of monomorphizations that can be shared across crates via `-Zshare-generics`. This version of the PR might have detrimental effects on performance as it makes lots of stuff dependent on a single query results (`upstream_monomorphizations_for(def_id_of_drop_in_place)`). That should be fixable but let's do a perf run first. Potentially fixes issue #64140. (cc @alexcrichton) The changes here are related to @matthewjasper's #67332 but should be mostly orthogonal. r? @ghost
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
☀️ Try build successful - checks-azure |
Queued d9ce420 with parent 2f688ac, future comparison URL. |
Alright, I found the bug. Turns out |
Finished benchmarking try commit d9ce420, comparison URL. |
used outside of the LLVM backend.
9494974
to
d3ca81c
Compare
This is ready for an actual review. |
r? @alexcrichton ( |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
d3ca81c
to
168c6a1
Compare
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
168c6a1
to
7bbdeb6
Compare
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.
While I don't think I understand fully all the intricacies of what's going on here it all looks reasonable enough to me. Just one thing to double-check below and otherwise r=me
This reduces the amount of invalidated data when new types are add to upstream crates.
7bbdeb6
to
197cc1e
Compare
@bors r=alexcrichton Thanks for the review! |
📌 Commit 197cc1e has been approved by |
@bors rollup=never |
Also share drop-glue when compiling with -Zshare-generics (i.e. at opt-level=0) This PR adds drop-glue to the set of monomorphizations that can be shared across crates via `-Zshare-generics`. This version of the PR might have detrimental effects on performance as it makes lots of stuff dependent on a single query results (`upstream_monomorphizations_for(def_id_of_drop_in_place)`). That should be fixable but let's do a perf run first. Potentially fixes issue #64140. (cc @alexcrichton) The changes here are related to @matthewjasper's #67332 but should be mostly orthogonal. r? @ghost
☀️ Test successful - checks-azure |
This PR adds drop-glue to the set of monomorphizations that can be shared across crates via
-Zshare-generics
.This version of the PR might have detrimental effects on performance as it makes lots of stuff dependent on a single query results (
upstream_monomorphizations_for(def_id_of_drop_in_place)
). That should be fixable but let's do a perf run first.Potentially fixes issue #64140. (cc @alexcrichton)
The changes here are related to @matthewjasper's #67332 but should be mostly orthogonal.
r? @ghost