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

Rewrite `precompute_borrows_out_of_scope` for fewer hash table lookups. #53942

Merged
merged 1 commit into from Sep 8, 2018

Conversation

Projects
None yet
9 participants
@nnethercote
Copy link
Contributor

nnethercote commented Sep 4, 2018

It now does one hash table lookup per basic block, instead of one per
statement. This is worthwhile because this function is hot for NLL
builds of ucd.

I haven't measured the effect of this yet because I'm having trouble doing optimized builds of rustc that are suitable for profiling (#53916). I will do an online perf run instead.

r? @nikomatsakis

Rewrite `precompute_borrows_out_of_scope` for fewer hash table lookups.
It now does one hash table lookup per basic block, instead of one per
statement. This is worthwhile because this function is hot for NLL
builds of `ucd`.
@nnethercote

This comment has been minimized.

Copy link
Contributor Author

nnethercote commented Sep 4, 2018

@bors try

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Sep 4, 2018

⌛️ Trying commit fb307e5 with merge e658883...

bors added a commit that referenced this pull request Sep 4, 2018

Auto merge of #53942 - nnethercote:faster-precompute, r=<try>
Rewrite `precompute_borrows_out_of_scope` for fewer hash table lookups.

It now does one hash table lookup per basic block, instead of one per
statement. This is worthwhile because this function is hot for NLL
builds of `ucd`.

I haven't measured the effect of this yet because I'm having trouble doing optimized builds of rustc that are suitable for profiling (#53916). I will do an online perf run instead.

r? @nikomatsakis
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Sep 4, 2018

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

@nnethercote

This comment has been minimized.

Copy link
Contributor Author

nnethercote commented Sep 4, 2018

@rust-timer

This comment has been minimized.

Copy link

rust-timer commented Sep 4, 2018

Success: Queued e658883 with parent 8b80390, comparison URL.

@nnethercote

This comment has been minimized.

Copy link
Contributor Author

nnethercote commented Sep 4, 2018

BTW, I verified the correctness of this by implementing the new code alongside the old code, running both in tandem, and asserting that the HashSets of visited nodes matched each other.

@memoryruins memoryruins added the A-NLL label Sep 4, 2018

@ljedrz

This comment has been minimized.

Copy link
Contributor

ljedrz commented Sep 4, 2018

Perf results are in; ucd-check improves by almost 60%.

@nikomatsakis
Copy link
Contributor

nikomatsakis left a comment

This is nice.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Sep 4, 2018

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Sep 4, 2018

📌 Commit fb307e5 has been approved by nikomatsakis

@nnethercote

This comment has been minimized.

Copy link
Contributor Author

nnethercote commented Sep 4, 2018

Perf results are in; ucd-check improves by almost 60%.

Yay! And it's a 1% NLL win for html5ever-check, too.

@nikomatsakis: I have one question. Each BB has N statements, with indices 0..N, plus a terminator at index N. Currently, the terminator location gets tested by region_contains(), and I don't know if that is valid or not.

@nikomatsakis

This comment has been minimized.

Copy link
Contributor

nikomatsakis commented Sep 5, 2018

@nnethercote

Currently, the terminator location gets tested by region_contains(), and I don't know if that is valid or not.

This is correct, the terminator location is no different from the others from the POV of this code.

@nnethercote

This comment has been minimized.

Copy link
Contributor Author

nnethercote commented Sep 5, 2018

I bet $5 that #53909 will end up landing before this does, in which case this PR will provide (a) a 1% html5ever-check NLL win, and (b) future protection against other potential bad cases.

@pnkfelix

This comment has been minimized.

Copy link
Member

pnkfelix commented Sep 6, 2018

I bet $5 that #53909 will end up landing before this does, ...

well that is an interesting technique to try to get your PR some favoritism from the reviewers! 😉

@nnethercote

This comment has been minimized.

Copy link
Contributor Author

nnethercote commented Sep 6, 2018

48 hours ago this PR was 10th in Homu's queue. It's now 11th. I find Homu frustrating: #37107

kennytm added a commit to kennytm/rust that referenced this pull request Sep 8, 2018

Rollup merge of rust-lang#53942 - nnethercote:faster-precompute, r=ni…
…komatsakis

Rewrite `precompute_borrows_out_of_scope` for fewer hash table lookups.

It now does one hash table lookup per basic block, instead of one per
statement. This is worthwhile because this function is hot for NLL
builds of `ucd`.

I haven't measured the effect of this yet because I'm having trouble doing optimized builds of rustc that are suitable for profiling (rust-lang#53916). I will do an online perf run instead.

r? @nikomatsakis

bors added a commit that referenced this pull request Sep 8, 2018

Auto merge of #54051 - kennytm:rollup, r=kennytm
Rollup of 11 pull requests

Successful merges:

 - #51366 (stabilize #[panic_handler])
 - #53162 (rustdoc: collect trait impls as an early pass)
 - #53932 ([NLL] Remove base_place)
 - #53942 (Rewrite `precompute_borrows_out_of_scope` for fewer hash table lookups.)
 - #53973 (Have rust-lldb look for the rust-enabled lldb)
 - #53981 (Implement initializer() for FileDesc)
 - #53987 (rustbuild: allow configuring llvm version suffix)
 - #53993 (rustc_resolve: don't record uniform_paths canaries as reexports.)
 - #54007 (crates that provide a `panic_handler` are exempt from the `unused_extern_crates` lint)
 - #54040 (update books for next release)
 - #54050 (Update `petgraph` dependency to 0.4.13 to fix build with nightly)

Failed merges:

r? @ghost

kennytm added a commit to kennytm/rust that referenced this pull request Sep 8, 2018

Rollup merge of rust-lang#53942 - nnethercote:faster-precompute, r=ni…
…komatsakis

Rewrite `precompute_borrows_out_of_scope` for fewer hash table lookups.

It now does one hash table lookup per basic block, instead of one per
statement. This is worthwhile because this function is hot for NLL
builds of `ucd`.

I haven't measured the effect of this yet because I'm having trouble doing optimized builds of rustc that are suitable for profiling (rust-lang#53916). I will do an online perf run instead.

r? @nikomatsakis

bors added a commit that referenced this pull request Sep 8, 2018

Auto merge of #54051 - kennytm:rollup, r=kennytm
Rollup of 10 pull requests

Successful merges:

 - #53315 (use `NonZeroU32` in `newtype_index!`macro, change syntax)
 - #53932 ([NLL] Remove base_place)
 - #53942 (Rewrite `precompute_borrows_out_of_scope` for fewer hash table lookups.)
 - #53973 (Have rust-lldb look for the rust-enabled lldb)
 - #53981 (Implement initializer() for FileDesc)
 - #53987 (rustbuild: allow configuring llvm version suffix)
 - #53993 (rustc_resolve: don't record uniform_paths canaries as reexports.)
 - #54007 (crates that provide a `panic_handler` are exempt from the `unused_extern_crates` lint)
 - #54040 (update books for next release)
 - #54050 (Update `petgraph` dependency to 0.4.13 to fix build with nightly)

@bors bors merged commit fb307e5 into rust-lang:master Sep 8, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details

@nnethercote nnethercote deleted the nnethercote:faster-precompute branch Sep 9, 2018

@nnethercote

This comment has been minimized.

Copy link
Contributor Author

nnethercote commented Sep 10, 2018

I bet $5 that #53909 will end up landing before this does

It did, by about 4 hours.

@lqd

This comment has been minimized.

Copy link
Contributor

lqd commented Sep 10, 2018

@nnethercote it looks more like yours merged 8h before #53909 (in my timezone of GMT+2: 4PM Sept 8, while 53909 did on Sept 9 at 20 minutes after midnight :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.