perf: Revert accidental inclusion of a part of #69218 by Marwes · Pull Request #71996 · rust-lang/rust

Marwes · 2020-05-07T21:30:57Z

This was accidentally included in #69464 after a rebase and given
how much inflate and keccak stresses the obligation forest seems
like a likely culprit to the regression in those benchmarks.

(It is necessary in #69218 as obligation forest needs to accurately
track the root variables or unifications will get lost)

rust-highfive · 2020-05-07T21:31:00Z

r? @davidtwco

(rust_highfive has picked a reviewer for you, use r? to override)

This was accidentally included in rust-lang#69494 after a rebase and given how much `inflate` and `keccak` stresses the obligation forest seems like a likely culprit to the regression in those benchmarks. (It is necessary in rust-lang#69218 as obligation forest needs to accurately track the root variables or unifications will get lost)

jonas-schievink · 2020-05-07T21:34:35Z

#69494

That seems like the wrong PR

jonas-schievink · 2020-05-07T21:35:51Z

@bors try @rust-timer queue

rust-timer · 2020-05-07T21:35:52Z

Awaiting bors try build completion

bors · 2020-05-07T21:36:00Z

⌛ Trying commit 1d8489c with merge b80dbeb9b6dc6b59230376c05f5d2dd14a757f54...

bors · 2020-05-08T00:45:46Z

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

rust-timer · 2020-05-08T00:45:48Z

Queued b80dbeb9b6dc6b59230376c05f5d2dd14a757f54 with parent a08c473, future comparison URL.

nikomatsakis · 2020-05-08T13:03:22Z

@bors r+

the perf run results, however, aren't visible yet

bors · 2020-05-08T13:03:24Z

📌 Commit 1d8489c has been approved by nikomatsakis

nikomatsakis · 2020-05-08T13:03:44Z

@bors delegate+

bors · 2020-05-08T13:03:45Z

✌️ @Marwes can now approve this pull request

rust-timer · 2020-05-09T07:59:27Z

Finished benchmarking try commit b80dbeb9b6dc6b59230376c05f5d2dd14a757f54, comparison URL.

RalfJung · 2020-05-11T08:14:19Z

@bors rollup=never (please do that for PRs that are suspected to impact perf)

davidtwco · 2020-05-11T10:38:28Z

r? @nikomatsakis

nikomatsakis · 2020-05-11T14:07:26Z

@bors r-

Hmm

nikomatsakis · 2020-05-11T14:07:54Z

@Marwes doesn't seem to have much impact on keccak... any thoughts?

Marwes · 2020-05-12T18:58:58Z

Trying to figure that out but I really can't find another place which would disproportionately affect keccak and inflate :S

Reclaims most of the regression in inflate

Marwes · 2020-05-24T21:07:46Z

Adding some #[inline] fixed most of it with inflate only having regressed by 1.5% in my measurements.

@nnethercote I tried cachegrind but for whatever reason I can't get it to work. Tried with callgrind also and tried to view the output in kcachegrind (which I have used before) but I just get addresses displayed. I guess something makes valgrind/cachegrind/ etc not understand the debug information. (perf + hotspot works fine though, as usual :/)

nnethercote · 2020-05-24T22:55:58Z

@Marwes: your comment made me realize that the docs are missing a crucial part about setting a config.toml correctly. I just added a new section explaining it. Could you try again with that? Cachegrind is great for detecting inlining issues, because an inlined function has no instruction counts on its first and last lines, while a non-inline function does.

Marwes · 2020-05-25T06:25:04Z

Used debuginfo-level = 2 in my case which didn't work :/

nnethercote · 2020-05-25T07:26:30Z

Didn't work how? Can you show me some example output?

Marwes · 2020-05-25T07:57:42Z

Running valgrind --tool=cachegrind --cache-sim=no --branch-sim=no ~/Code/rust/build/x86_64-unknown-linux-gnu/stage1/bin/rustc --crate-type=lib src/lib.rs

And then running cg_diff on the output file just gives me a list of c/c++ functions, putting the same file into kcachegrind shows what I can only assume a are memory addresses display in lieu of debug info.

(Diffs a file with itself so everything is zero, still there are not rust functions in the diff)

fl=/build/glibc-OTsEL5/glibc-2.27/malloc/arena.c
fn=_int_free
0 0
fl=???
fn=std::ios_base::ios_base()
0 0
fl=/build/glibc-OTsEL5/glibc-2.27/posix/../sysdeps/unix/sysv/linux/_exit.c
fn=_Exit
0 0
fl=/build/glibc-OTsEL5/glibc-2.27/stdlib/msort.c
fn=qsort_r
0 0
fl=/build/glibc-OTsEL5/glibc-2.27/elf/dl-lookup.c
fn=_dl_setup_hash
0 0
fl=/build/glibc-OTsEL5/glibc-2.27/string/strdup.c
fn=strdup
0 0
fl=/build/glibc-OTsEL5/glibc-2.27/nptl/../include/list.h
fn=__free_tcb
0 0
fl=/build/glibc-OTsEL5/glibc-2.27/elf/../misc/sbrk.c
fn=sbrk
0 0
fl=/build/glibc-OTsEL5/glibc-2.27/string/../sysdeps/x86_64/multiarch/../strlen.S
fn=strlen
0 0
fl=/build/glibc-OTsEL5/glibc-2.27/string/../sysdeps/x86_64/multiarch/strcspn.c
fn=strcspn
0 0
fl=/build/glibc-OTsEL5/glibc-2.27/math/../sysdeps/x86_64/fpu/multiarch/s_rint.c
fn=rint
0 0
fl=???
fn=std::_Rb_tree_decrement(std::_Rb_tree_node_base*)
0 0
fl=/build/glibc-OTsEL5/glibc-2.27/csu/../sysdeps/unix/sysv/linux/x86_64/init-first.c
fn=_init
0 0
fl=/build/glibc-OTsEL5/glibc-2.27/nptl/../sysdeps/unix/sysv/linux/pthread-pids.h
fn=__pthread_initialize_minimal
0 0
fl=/build/glibc-OTsEL5/glibc-2.27/elf/dl-load.c
fn=_dl_map_object
0 0
fl=/build/glibc-OTsEL5/glibc-2.27/io/../sysdeps/unix/sysv/linux/poll.c
fn=poll
0 0
fl=/build/glibc-OTsEL5/glibc-2.27/wcsmbs/../sysdeps/x86_64/multiarch/ifunc-avx2.h
fn=wcslen
0 0
fl=/build/glibc-OTsEL5/glibc-2.27/posix/../sysdeps/unix/syscall-template.S
fn=getpid
0 0
fl=???
fn=std::ostream::flush()
0 0
fl=/build/glibc-OTsEL5/glibc-2.27/nptl/../sysdeps/unix/sysv/linux/x86_64/cancellation.S
fn=__libc_disable_asynccancel
0 0
fl=/build/glibc-OTsEL5/glibc-2.27/malloc/malloc.c
fn=_int_realloc
0 0
summary: 0

Mark-Simulacrum · 2020-05-25T10:36:59Z

Note that I'm not sure we'll rebuild the compiler for you, so you may need to x.py clean and rebuild.

Also, debuginfo level of 1 should be sufficient I suspect and significantly lighter on disk usage - I've personally seen IIRC tens of gigabytes of space savings.

nnethercote · 2020-05-25T10:41:33Z

I also recommend running Cachegrind via collector, rather than directly. I find it simpler that way.

Marwes · 2020-05-26T14:36:06Z

Cleaning and rebuilding does not help, nor does just using level 1 or running via collector.

Could use a perf run to validate that inlining helps.

nikomatsakis · 2020-05-26T14:40:36Z

@bors try @rust-timer queue

rust-timer · 2020-05-26T14:40:37Z

Awaiting bors try build completion

bors · 2020-05-26T14:40:52Z

⌛ Trying commit ebc7eda with merge ad74291237c3c0233ace59eec9db124ff1c17082...

bors · 2020-05-26T17:33:19Z

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

rust-timer · 2020-05-26T17:33:20Z

Queued ad74291237c3c0233ace59eec9db124ff1c17082 with parent aeca4d6, future comparison URL.

rust-timer · 2020-05-26T20:35:22Z

Finished benchmarking try commit ad74291237c3c0233ace59eec9db124ff1c17082, comparison URL.

Aaron1011 · 2020-05-26T20:46:27Z

Great work @Marwes!

Marwes · 2020-05-26T20:48:49Z

It doesn't recover all the performance but it is at least most of it (1.5% regression remaining locally). However with #69218 it probably doesn't matter anyway, since that improves keccak and inflate by so much that it won't really be noticed.

nikomatsakis · 2020-05-27T14:39:43Z

@bors r+ rollup=neve

bors · 2020-05-27T14:39:44Z

📌 Commit ebc7eda has been approved by nikomatsakis

nikomatsakis · 2020-05-27T14:39:48Z

@bors rollup=never

bors · 2020-05-27T18:31:51Z

⌛ Testing commit ebc7eda with merge 664fcd3...

bors · 2020-05-27T22:23:35Z

☀️ Test successful - checks-azure
Approved by: nikomatsakis
Pushing 664fcd3 to master...

nnethercote · 2020-06-01T23:32:15Z

The perf improvements from the landing are here. Nice work!

rust-highfive assigned davidtwco May 7, 2020

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 7, 2020

Marwes force-pushed the detach_undo_log branch from 100bccc to 1d8489c Compare May 7, 2020 21:31

Marwes mentioned this pull request May 7, 2020

perf: Unify the undo log of all snapshot types #69464

Merged

Marwes mentioned this pull request May 8, 2020

perf: Only process changed obligations in ObligationForest #69218

Closed

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 May 8, 2020

rust-highfive assigned nikomatsakis and unassigned davidtwco May 11, 2020

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels May 11, 2020

perf: Add inline on commonly used methods added in 69464

ebc7eda

Reclaims most of the regression in inflate

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels May 27, 2020

bors added the merged-by-bors This PR was explicitly merged by bors. label May 27, 2020

bors merged commit 664fcd3 into rust-lang:master May 27, 2020

bors mentioned this pull request May 27, 2020

Rename traits::Vtable to ImplSource. #72114

Merged

Marwes deleted the detach_undo_log branch May 28, 2020 06:09

Uh oh!

Conversation

Marwes commented May 7, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rust-highfive commented May 7, 2020

Uh oh!

jonas-schievink commented May 7, 2020

Uh oh!

jonas-schievink commented May 7, 2020

Uh oh!

rust-timer commented May 7, 2020

Uh oh!

bors commented May 7, 2020

Uh oh!

bors commented May 8, 2020

Uh oh!

rust-timer commented May 8, 2020

Uh oh!

nikomatsakis commented May 8, 2020

Uh oh!

bors commented May 8, 2020

Uh oh!

nikomatsakis commented May 8, 2020

Uh oh!

bors commented May 8, 2020

Uh oh!

rust-timer commented May 9, 2020

Uh oh!

RalfJung commented May 11, 2020

Uh oh!

davidtwco commented May 11, 2020

Uh oh!

nikomatsakis commented May 11, 2020

Uh oh!

nikomatsakis commented May 11, 2020

Uh oh!

Marwes commented May 12, 2020

Uh oh!

Marwes commented May 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nnethercote commented May 24, 2020

Uh oh!

Marwes commented May 25, 2020

Uh oh!

nnethercote commented May 25, 2020

Uh oh!

Marwes commented May 25, 2020

Uh oh!

Mark-Simulacrum commented May 25, 2020

Uh oh!

nnethercote commented May 25, 2020

Uh oh!

Marwes commented May 26, 2020

Uh oh!

nikomatsakis commented May 26, 2020

Uh oh!

rust-timer commented May 26, 2020

Uh oh!

bors commented May 26, 2020

Uh oh!

bors commented May 26, 2020

Uh oh!

rust-timer commented May 26, 2020

Uh oh!

rust-timer commented May 26, 2020

Uh oh!

Aaron1011 commented May 26, 2020

Uh oh!

Marwes commented May 26, 2020

Uh oh!

nikomatsakis commented May 27, 2020

Uh oh!

bors commented May 27, 2020

Uh oh!

nikomatsakis commented May 27, 2020

Uh oh!

bors commented May 27, 2020

Uh oh!

Marwes commented May 7, 2020 •

edited

Loading

Marwes commented May 24, 2020 •

edited

Loading