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

test caching opt_const_param_of on disc #74376

Merged
merged 1 commit into from
Jul 21, 2020

Conversation

lcnr
Copy link
Contributor

@lcnr lcnr commented Jul 15, 2020

Followup to #74113, implements parts of #74360

Tried caching opt_const_param_of on disk and adding an early exit if tcx.dep_kind(def_id) != DefKind::AnonConst.

Ended up causing a perf regression instead, so we just remove the FIXME and a short note to opt_const_param_of.

r? @eddyb

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jul 15, 2020
@lcnr
Copy link
Contributor Author

lcnr commented Jul 15, 2020

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Contributor

bors commented Jul 15, 2020

⌛ Trying commit 620d6883fd74efedbb67ac019186c6e76c826ea5 with merge 2e498501f591a1be258a953ea1ed225408909730...

@bors
Copy link
Contributor

bors commented Jul 15, 2020

☀️ Try build successful - checks-actions, checks-azure
Build commit: 2e498501f591a1be258a953ea1ed225408909730 (2e498501f591a1be258a953ea1ed225408909730)

@rust-timer
Copy link
Collaborator

Queued 2e498501f591a1be258a953ea1ed225408909730 with parent 7e11379, future comparison URL.

Copy link
Member

@eddyb eddyb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is fine, curious if perf will change.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (2e498501f591a1be258a953ea1ed225408909730): comparison url.

@lcnr
Copy link
Contributor Author

lcnr commented Jul 16, 2020

Looks like perf is worse 🤔 I think DefKind might actually increase the incremental deps, as opt_const_param_of currently only looks into Node::AnonConst. Let's try it without using DefKind.

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Contributor

bors commented Jul 16, 2020

⌛ Trying commit 016205678946e3483aac1bd53417ac3c8d867296 with merge 374482ebbce140f884a62148e2a93c7ceee04adf...

@bors
Copy link
Contributor

bors commented Jul 16, 2020

☀️ Try build successful - checks-actions, checks-azure
Build commit: 374482ebbce140f884a62148e2a93c7ceee04adf (374482ebbce140f884a62148e2a93c7ceee04adf)

@rust-timer
Copy link
Collaborator

Queued 374482ebbce140f884a62148e2a93c7ceee04adf with parent e2e29de, future comparison URL.

@eddyb
Copy link
Member

eddyb commented Jul 16, 2020

Heh, at least all the regressions are in non-initial incremental runs.

I think DefKind might actually increase the incremental deps, as opt_const_param_of currently only looks into Node::AnonConst

But when you request a HIR node, you depend on everything in it (I guess not on the body of things that have bodies, so maybe it's fine - it's similar to type_of or fn_sig in that way).

@lcnr
Copy link
Contributor Author

lcnr commented Jul 16, 2020

def_kind also always requests a Hir node. def_kind also does stuff with other Nodes which should increase the complexity of the dependency tree of opt_const_param_of.

@eddyb
Copy link
Member

eddyb commented Jul 16, 2020

@lcnr the cost comes from tcx.hir().get(...), it doesn't matter what you access inside, the fact that you got the Node means whatever you're in will get reexecuted if anything in that HIR owner (but outside bodies I guess) changes.

def_kind acts like a "firewall", in that if opt_const_param_of uses it for the early return, opt_const_param_of will only be recomputed if def_kind's value changes.

E.g. opt_const_param_of(some_function) would normally get recomputed when the signature changes, but with the def_kind early-exit in place, it won't be recomputed when the signature changes because the value of def_kind(some_function) hadn't changed (it returns DefKind::Fn both times, and the second time opt_const_param_of isn't even executed).

But apparently it's not worth it, i.e. recomputing opt_const_param_of (when it returns None) is cheap enough, and I guess that makes sense.

Although, I wonder if the perf results are from caching on disk, that seems like it would matter more than the early return.

@lcnr
Copy link
Contributor Author

lcnr commented Jul 16, 2020

My reasoning is that def_kind calls get_parent_node of Node::Ctor. So if the parent_node changes for def_kind get's invalidated while the current opt_const_param_of does not.

I kind of doubt that that's relevant here though, the perf impact probably just comes from caching it on disc at all 😅

@eddyb
Copy link
Member

eddyb commented Jul 16, 2020

def_kind being invalidated is fine, because that won't invalidate opt_const_param_of unless the value changes.
That is, dependencies are not transitive, every step of the way can block the propagation of changes.
This is also known as "red-green" incremental.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (374482ebbce140f884a62148e2a93c7ceee04adf): comparison url.

@lcnr
Copy link
Contributor Author

lcnr commented Jul 16, 2020

🤷 let's just add a note to opt_const_param_of and call it a day here 😆

@lcnr lcnr force-pushed the type-dependent-path-cleanup branch from 0162056 to 57fc3d3 Compare July 18, 2020 17:22
@lcnr
Copy link
Contributor Author

lcnr commented Jul 18, 2020

ready for merge I guess

@lcnr lcnr force-pushed the type-dependent-path-cleanup branch from 57fc3d3 to e8d16fd Compare July 18, 2020 17:23
Copy link
Member

@eddyb eddyb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

r=me with title/description updated to match the current state

@lcnr lcnr changed the title cache opt_const_param_of on disc test caching opt_const_param_of on disc Jul 20, 2020
@eddyb
Copy link
Member

eddyb commented Jul 20, 2020

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Jul 20, 2020

📌 Commit e8d16fd has been approved by eddyb

@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 Jul 20, 2020
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 20, 2020
…arth

Rollup of 13 pull requests

Successful merges:

 - rust-lang#72714 (Fix debug assertion in typeck)
 - rust-lang#73197 (Impl Default for ranges)
 - rust-lang#73323 (wf: check foreign fn decls for well-formedness)
 - rust-lang#74051 (disallow non-static lifetimes in const generics)
 - rust-lang#74376 (test caching opt_const_param_of on disc)
 - rust-lang#74501 (Ayu theme: Use different background color for Run button)
 - rust-lang#74505 (Fix search input focus in ayu theme)
 - rust-lang#74522 (Update sanitizer docs)
 - rust-lang#74546 (Fix duplicate maybe_uninit_extra attribute)
 - rust-lang#74552 (Stabilize TAU constant.)
 - rust-lang#74555 (Improve "important traits" popup display on mobile)
 - rust-lang#74557 (Fix an ICE on an invalid `binding @ ...` in a tuple struct pattern)
 - rust-lang#74561 (update backtrace-rs)

Failed merges:

r? @ghost
@bors bors merged commit 6a4276d into rust-lang:master Jul 21, 2020
@lcnr lcnr deleted the type-dependent-path-cleanup branch July 21, 2020 06:51
bors added a commit to rust-lang-ci/rust that referenced this pull request Jul 22, 2020
remove some const arg in ty dep path boilerplate

followup to rust-lang#74113, together with rust-lang#74376, this closes rust-lang#74360.

r? @eddyb
@cuviper cuviper added this to the 1.47.0 milestone May 2, 2024
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants