Move query code outside macros and store query jobs separately from query results#50102
Move query code outside macros and store query jobs separately from query results#50102bors merged 4 commits intorust-lang:masterfrom
Conversation
emilyalbini
added
T-compiler
|
Thanks, @Zoxc! Do you have any measurements how splitting the map affects memory usage and performance? In the cache miss case, it leads to a few more hashtable lookups than the current version, I think. |
|
It uses 450MB when compiling syntex_syntax vs. 456MB before. Compile times seems to be slightly reduced, although that may just be due to noise. |
|
Ping from triage @michaelwoerister ! This PR needs your review. |
|
@bors try |
|
⌛ Trying commit 87956b5a5eaee00b89297551c1dd75e1ff632650 with merge 34c50be0a1c561e15f7d02e06c7e47b48a40e444... |
|
☀️ Test successful - status-travis |
|
@Mark-Simulacrum I'd like a perf run here |
|
Perf queued. |
|
I did some microbenchmarks here too: After: Before (with incremental compilation): After (with incremental compilation): |
|
Looks very promising, @Zoxc! |
|
I think I'm going to close #50067 in favor of this. |
|
Thanks, @Zoxc! The first two commits look great. The third one I'm a little ambiguous about because of the additional table lookups. But since performance seems to improve overall, I don't have any real objections. All in all this PR cleans up the core query code considerably! No we just have to merge r=me with the comments from #50067 addressed. |
|
@bors r=michaelwoerister |
|
📌 Commit f678bf0 has been approved by |
bors
added
S-waiting-on-bors
|
🎉 |
Move query code outside macros and store query jobs separately from query results Based on #50067 r? @michaelwoerister
|
☀️ Test successful - status-appveyor, status-travis |
…hercote Rename `QueryResult` to `ActiveKeyStatus` Long ago, this enum was also used to hold the cached result of a query evaluation that had completed successfully, so its name made some sense. Nowadays, successful query evaluation is represented by an entry in the query's in-memory cache, which is a separate data structure. So this enum only deals with “active” query keys, i.e. those for which query evaluation has started, but has not yet completed successfully. --- The split between jobs and results was introduced by: - rust-lang#50102. --- There should be no change to compiler behaviour.
…hercote Rename `QueryResult` to `ActiveKeyStatus` Long ago, this enum was also used to hold the cached result of a query evaluation that had completed successfully, so its name made some sense. Nowadays, successful query evaluation is represented by an entry in the query's in-memory cache, which is a separate data structure. So this enum only deals with “active” query keys, i.e. those for which query evaluation has started, but has not yet completed successfully. --- The split between jobs and results was introduced by: - rust-lang#50102. --- There should be no change to compiler behaviour.
Rollup merge of #151907 - Zalathar:active-key-status, r=nnethercote Rename `QueryResult` to `ActiveKeyStatus` Long ago, this enum was also used to hold the cached result of a query evaluation that had completed successfully, so its name made some sense. Nowadays, successful query evaluation is represented by an entry in the query's in-memory cache, which is a separate data structure. So this enum only deals with “active” query keys, i.e. those for which query evaluation has started, but has not yet completed successfully. --- The split between jobs and results was introduced by: - #50102. --- There should be no change to compiler behaviour.
6 participants
Based on #50067
r? @michaelwoerister