Create a job immediately when looking in the query map and start it later #50067
Conversation
|
@bors try |
|
⌛ Trying commit c8191a2 with merge a7eed60bdc55f5778f483805dc13edef027ba7a3... |
|
☀️ Test successful - status-travis |
|
@Mark-Simulacrum I'd like a perf run here too |
|
Looks great! I'll try to do a real review later today. |
There was a problem hiding this comment.
Thanks, @Zoxc! That's much nicer, I think. Let's wait for the perf results, but except for that r=me. The librustc compile time reduction is great too!
| // when accessing the ImplicitCtxt | ||
| let r = tls::with_related_context(tcx, move |icx| { | ||
| // Update the ImplicitCtxt to point to our new query job | ||
| let icx = tls::ImplicitCtxt { |
There was a problem hiding this comment.
Could you rename this to new_icx and the other one to current_icx or something like that? That would make it clearer.
| type Value: Clone; | ||
|
|
||
| fn query(key: Self::Key) -> Query<'tcx>; | ||
| fn query_map<'a>(tcx: TyCtxt<'a, 'tcx, '_>) -> &'a Lock<QueryMap<'tcx, Self>>; |
There was a problem hiding this comment.
Maybe add a little word of warning here to not use this directly.
pietroalbini
added
the
S-waiting-on-author
|
Overall this approach seems to be slightly slower: I wonder why that is. From the diff it looks like performance should be the same. |
|
I wrote some microbenchmarks for queries. Before: After changes in this PR + moving code out from macros: |
|
Before (with incremental compilation): After (with incremental compilation): The |
|
OK, I can see that we are doing more work for invocations that then hit the on-disk cache because we are allocating the job unconditionally. I'm still surprised about the difference in the non-incremental case. |
|
I changed the benchmark to use a Now it seems to be a speedup all cases: |
OK, that's encouraging. What's the status of this PR. Do you want to close it in favor of #50102? |
|
I'd like to find out why this is slower for |
Move query code outside macros and store query jobs separately from query results Based on #50067 r? @michaelwoerister
This means we only look up the key in the query map once, and the query map lock is held minimally.
This also moves some function outside the query macro which has the benefit of improving
librustccompile time.Before: 730.189 seconds
After: 659.451 seconds
Which means it saves about 70.738 seconds, which isn't too bad.
r? @michaelwoerister