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

Move query code outside macros and store query jobs separately from query results #50102

Merged
merged 4 commits into from
Apr 27, 2018

Conversation

Zoxc
Copy link
Contributor

@Zoxc Zoxc commented Apr 20, 2018

@pietroalbini pietroalbini added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 20, 2018
@michaelwoerister
Copy link
Member

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.

@Zoxc
Copy link
Contributor Author

Zoxc commented Apr 20, 2018

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.

@TimNN
Copy link
Contributor

TimNN commented Apr 24, 2018

Ping from triage @michaelwoerister ! This PR needs your review.

@Zoxc
Copy link
Contributor Author

Zoxc commented Apr 25, 2018

@bors try

@bors
Copy link
Contributor

bors commented Apr 25, 2018

⌛ Trying commit 87956b5a5eaee00b89297551c1dd75e1ff632650 with merge 34c50be0a1c561e15f7d02e06c7e47b48a40e444...

@bors
Copy link
Contributor

bors commented Apr 25, 2018

☀️ Test successful - status-travis
State: approved= try=True

@Zoxc
Copy link
Contributor Author

Zoxc commented Apr 25, 2018

@Mark-Simulacrum I'd like a perf run here

@Mark-Simulacrum
Copy link
Member

Perf queued.

@Zoxc
Copy link
Contributor Author

Zoxc commented Apr 26, 2018

I did some microbenchmarks here too:
Before:

  time: 0.827; rss: 36MB        hot query (usize)
  time: 0.942; rss: 170MB       cold query (usize)
  time: 0.911; rss: 170MB       hot query (dummy DefId)
  time: 0.972; rss: 304MB       cold query (dummy DefId)
  time: 0.960; rss: 304MB       hot query (DefId)
  time: 0.953; rss: 438MB       cold query (DefId)

After:

  time: 0.367; rss: 36MB        hot query (usize)
  time: 0.717; rss: 136MB       cold query (usize)
  time: 0.498; rss: 136MB       hot query (DefId)
  time: 0.795; rss: 237MB       cold query (DefId)
  time: 0.379; rss: 237MB       hot query (dummy DefId)
  time: 0.706; rss: 338MB       cold query (dummy DefId)

Before (with incremental compilation):

  time: 0.958; rss: 40MB        hot query (usize)
  time: 3.165; rss: 638MB       cold query (usize)
  time: 1.080; rss: 638MB       hot query (dummy DefId)
  time: 3.286; rss: 1237MB      cold query (dummy DefId)
  time: 1.088; rss: 1237MB      hot query (DefId)

After (with incremental compilation):

  time: 0.511; rss: 40MB        hot query (usize)
  time: 2.584; rss: 605MB       cold query (usize)
  time: 0.680; rss: 605MB       hot query (DefId)
  time: 0.478; rss: 605MB       hot query (dummy DefId)
  time: 2.796; rss: 1170MB      cold query (dummy DefId)

@michaelwoerister
Copy link
Member

Looks very promising, @Zoxc!

@Zoxc
Copy link
Contributor Author

Zoxc commented Apr 27, 2018

@michaelwoerister
Copy link
Member

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 dep_node.rs into plumbing.rs and all will be great in query land ;)

r=me with the comments from #50067 addressed.

@Zoxc
Copy link
Contributor Author

Zoxc commented Apr 27, 2018

@bors r=michaelwoerister

@bors
Copy link
Contributor

bors commented Apr 27, 2018

📌 Commit f678bf0 has been approved by michaelwoerister

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Apr 27, 2018
@michaelwoerister
Copy link
Member

🎉

@bors
Copy link
Contributor

bors commented Apr 27, 2018

⌛ Testing commit f678bf0 with merge 3c43aa5...

bors added a commit that referenced this pull request Apr 27, 2018
Move query code outside macros and store query jobs separately from query results

Based on #50067

r? @michaelwoerister
@bors
Copy link
Contributor

bors commented Apr 27, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: michaelwoerister
Pushing 3c43aa5 to master...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants