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.

Sign up for GitHub

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

Jump to bottom

Remove inlines from DepNode code #64850

Merged
merged 1 commit into from
Oct 2, 2019
Merged

Remove inlines from DepNode code #64850

merged 1 commit into from
Oct 2, 2019

Conversation

Mark-Simulacrum
Copy link
Member

@Mark-Simulacrum Mark-Simulacrum commented Sep 27, 2019 •
edited
Loading

No description provided.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 27, 2019
@Mark-Simulacrum
Copy link
Member Author

@bors try @rust-timer queue

Let's also get perf stats to see if this is a compile time regression (though I would be surprised).

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Contributor

bors commented Sep 27, 2019

⌛ Trying commit d3bc8d0 with merge 450cce3...

bors added a commit that referenced this pull request Sep 27, 2019
@bors
Auto merge of #64850 - Mark-Simulacrum:dedup-dep-node, r=<try>
450cce3 
Remove duplicate DepNode::new

Locally this shaves off about 3 seconds of compile time for librustc, which while not much, is something for a simple PR like this.

Code is merely duplicated up from the main `DepNode::new` body.

Across 3 runs for each branch I obtained these times for compiling librustc:
master: 286.244, 285.208, 303.577
this branch: 282.175, 281.891, 282.968

r? @michaelwoerister
@bors
Copy link
Contributor

bors commented Sep 27, 2019

☀️ Try build successful - checks-azure
Build commit: 450cce3 (450cce39ff25f888d9cf567c1e4aa844f41db521)

@rust-timer
Copy link
Collaborator

Queued 450cce3 with parent a37fe2d, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit 450cce3, comparison URL.

@michaelwoerister
Copy link
Member

I'd like to see what happens if we just remove #[inline(always)] annotation from DepNode::new(). I'm not a big fan of duplicating the code like that.

@Mark-Simulacrum
Copy link
Member Author

@bors try @rust-timer queue

Let's get this kicked off as well...

I've tried blanket removing inline from the dep_node module.

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Contributor

bors commented Sep 30, 2019

⌛ Trying commit 4857ee89d91d1b1cd96a071d9619cb24ebdc2cb6 with merge ddb12b8f1a4657e131bbab0b05082486df133247...

@Mark-Simulacrum
Copy link
Member Author

@bors retry

@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 Sep 30, 2019
@Mark-Simulacrum
Copy link
Member Author

@bors try

@bors
Copy link
Contributor

bors commented Sep 30, 2019

⌛ Trying commit 4107eda9350b9411a1768481da630ce245993b5a with merge 27680516ab5a239dfbc056ead5365098dff635dd...

@bors
Copy link
Contributor

bors commented Sep 30, 2019

☀️ Try build successful - checks-azure
Build commit: 27680516ab5a239dfbc056ead5365098dff635dd (27680516ab5a239dfbc056ead5365098dff635dd)

@rust-timer
Copy link
Collaborator

Queued 27680516ab5a239dfbc056ead5365098dff635dd with parent 22bc9e1, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit 27680516ab5a239dfbc056ead5365098dff635dd, comparison URL.

@michaelwoerister
Copy link
Member

The comparison doesn't show anything for me.

I think we should also mark the following functions as const (as suggested on Zulip):

  • can_reconstruct_query_key
  • is_anon
  • is_eval_always
  • has_params
  • from_def_path_hash (no sure if debug_assert is a problem for const-fn)
  • new_no_params (ditto)

@Mark-Simulacrum
Copy link
Member Author

Compare pages are a bit slower to render, I usually need to wait 1-2 seconds I think these days. It's on my todo list :)

I'll investigate const fn for those; I sort of expect it won't be possible -- I thought match, like if, is not yet supported in const fn.

@michaelwoerister
Copy link
Member

I'll investigate const fn for those; I sort of expect it won't be possible -- I thought match, like if, is not yet supported in const fn.

I thought MIRI would take care of those but apparently not yet.

@Mark-Simulacrum
Copy link
Member Author

I think I confirmed all of these can't be const fn due to insufficient work on const fn; miri probably can evaluate these but we haven't pushed that support into rustc const eval proper just yet.

@Mark-Simulacrum Mark-Simulacrum changed the title Remove duplicate DepNode::new Remove inlines from DepNode code Oct 1, 2019
michaelwoerister
michaelwoerister approved these changes Oct 2, 2019
View reviewed changes
Copy link
Member

@michaelwoerister michaelwoerister 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 I confirmed all of these can't be const fn due to insufficient work on const fn; miri probably can evaluate these but we haven't pushed that support into rustc const eval proper just yet.

That's a pity because almost all invocations have constant parameters. In the future maybe.

Thanks for the PR, @Mark-Simulacrum! r=me with the indentation fixed.

src/librustc/dep_graph/dep_node.rs Outdated
!dep_node.kind.can_reconstruct_query_key() &&
#[cfg(debug_assertions)]
{
if !dep_node.kind.can_reconstruct_query_key() &&
Copy link
Member

Choose a reason for hiding this comment

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

Indent please.

src/librustc/dep_graph/dep_node.rs Outdated
!dep_node.kind.can_reconstruct_query_key() &&
#[cfg(debug_assertions)]
{
if !dep_node.kind.can_reconstruct_query_key() &&
Copy link
Member

Choose a reason for hiding this comment

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

This too.

@Mark-Simulacrum
Copy link
Member Author

@bors r=michaelwoerister

Yeah, it is a bit unfortunate.

@bors
Copy link
Contributor

bors commented Oct 2, 2019

📌 Commit 675ed48 has been approved by michaelwoerister

Centril added a commit to Centril/rust that referenced this pull request Oct 2, 2019
@Centril
Rollup merge of rust-lang#64850 - Mark-Simulacrum:dedup-dep-node, r=m…
b7290a0 
…ichaelwoerister

Remove inlines from DepNode code
@Centril Centril mentioned this pull request Oct 2, 2019
Merged
bors added a commit that referenced this pull request Oct 2, 2019
@bors
Auto merge of #65009 - Centril:rollup-06g05xj, r=Centril
2daa404 
Rollup of 13 pull requests

Successful merges:

 - #64581 (Fix unreachable_code warnings for try{} block ok-wrapped expressions)
 - #64850 (Remove inlines from DepNode code)
 - #64914 (regression test for 64453 borrow check error.)
 - #64922 (Use PlaceBuilder to avoid a lot of slice -> vec -> slice convertions)
 - #64948 (Improve sidebar styling to make its integration easier)
 - #64961 (Make comment about dummy type a bit more clear)
 - #64967 (Don't mark borrows of zero-sized arrays as indirectly mutable)
 - #64973 (Fix typo while setting `compile-flags` in test)
 - #64980 (Enable support for `IndirectlyMutableLocals` in `rustc_peek` )
 - #64989 (Fix ICE #64964)
 - #64991 ([const-prop] Correctly handle locals that can't be propagated)
 - #64995 (Remove rustdoc warning)
 - #64997 (rustc book: nitpick SLP vectorization)

Failed merges:

r? @ghost
@bors bors merged commit 675ed48 into rust-lang:master Oct 2, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@michaelwoerister michaelwoerister michaelwoerister approved these changes

Assignees

@michaelwoerister michaelwoerister

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@Mark-Simulacrum @rust-timer @bors @michaelwoerister @rust-highfive