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

Make the arena fast path smaller #56404

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
6 participants
@Zoxc
Contributor

Zoxc commented Dec 1, 2018

@Zoxc

This comment has been minimized.

Contributor

Zoxc commented Dec 1, 2018

@bors try

@bors

This comment has been minimized.

Contributor

bors commented Dec 1, 2018

⌛️ Trying commit 45da76e with merge b40fd9a...

bors added a commit that referenced this pull request Dec 1, 2018

Auto merge of #56404 - Zoxc:arena-tweaks, r=<try>
Make the arena fast path smaller

r? @michaelwoerister
@rust-highfive

This comment was marked as outdated.

Collaborator

rust-highfive commented Dec 1, 2018

The job x86_64-gnu-llvm-5.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
travis_time:end:015edb22:start=1543662378846253076,finish=1543662379852852386,duration=1006599310
$ git checkout -qf FETCH_HEAD
travis_fold:end:git.checkout

Encrypted environment variables have been removed for security reasons.
See https://docs.travis-ci.com/user/pull-requests/#Pull-Requests-and-Security-Restrictions
$ export SCCACHE_BUCKET=rust-lang-ci-sccache2
$ export SCCACHE_REGION=us-west-1
Setting environment variables from .travis.yml
$ export IMAGE=x86_64-gnu-llvm-5.0
---

[00:03:19] travis_fold:start:tidy
travis_time:start:tidy
tidy check
[00:03:19] tidy error: /checkout/src/libarena/lib.rs:339: trailing whitespace
[00:03:21] some tidy checks failed
[00:03:21] 
[00:03:21] 
[00:03:21] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/tidy" "/checkout/src" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "--no-vendor" "--quiet"
[00:03:21] 
[00:03:21] 
[00:03:21] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
[00:03:21] Build completed unsuccessfully in 0:00:58
[00:03:21] Build completed unsuccessfully in 0:00:58
[00:03:21] Makefile:79: recipe for target 'tidy' failed
[00:03:21] make: *** [tidy] Error 1
The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:09ce7a30
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
Sat Dec  1 11:09:50 UTC 2018
---
travis_time:end:08a3088c:start=1543662591033605568,finish=1543662591038882936,duration=5277368
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:00d17ae4
$ ln -s . checkout && for CORE in obj/cores/core.*; do EXE=$(echo $CORE | sed 's|obj/cores/core\.[0-9]*\.!checkout!\(.*\)|\1|;y|!|/|'); if [ -f "$EXE" ]; then printf travis_fold":start:crashlog\n\033[31;1m%s\033[0m\n" "$CORE"; gdb --batch -q -c "$CORE" "$EXE" -iex 'set auto-load off' -iex 'dir src/' -iex 'set sysroot .' -ex bt -ex q; echo travis_fold":"end:crashlog; fi; done || true
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:033a0268
travis_time:start:033a0268
$ cat ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
cat: ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers: No such file or directory
travis_fold:end:after_failure.5
travis_fold:start:after_failure.6
travis_time:start:28245fe8
$ dmesg | grep -i kill

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@bors

This comment has been minimized.

Contributor

bors commented Dec 1, 2018

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

@Zoxc

This comment has been minimized.

Contributor

Zoxc commented Dec 1, 2018

@rust-timer

This comment has been minimized.

rust-timer commented Dec 1, 2018

Success: Queued b40fd9a with parent d3ed348, comparison URL.

@rust-timer

This comment has been minimized.

rust-timer commented Dec 1, 2018

Finished benchmarking try commit b40fd9a

@Zoxc Zoxc force-pushed the Zoxc:arena-tweaks branch from 45da76e to d052736 Dec 1, 2018

@rust-highfive

This comment was marked as outdated.

Collaborator

rust-highfive commented Dec 1, 2018

The job x86_64-gnu-llvm-5.0 of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
travis_time:end:057d6572:start=1543700241025187896,finish=1543700242064237774,duration=1039049878
$ git checkout -qf FETCH_HEAD
travis_fold:end:git.checkout

Encrypted environment variables have been removed for security reasons.
See https://docs.travis-ci.com/user/pull-requests/#Pull-Requests-and-Security-Restrictions
$ export SCCACHE_BUCKET=rust-lang-ci-sccache2
$ export SCCACHE_REGION=us-west-1
Setting environment variables from .travis.yml
$ export IMAGE=x86_64-gnu-llvm-5.0
---

[00:03:24] travis_fold:start:tidy
travis_time:start:tidy
tidy check
[00:03:24] tidy error: /checkout/src/libarena/lib.rs:368: trailing whitespace
[00:03:25] some tidy checks failed
[00:03:25] 
[00:03:25] 
[00:03:25] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/tidy" "/checkout/src" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "--no-vendor" "--quiet"
[00:03:25] 
[00:03:25] 
[00:03:25] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
[00:03:25] Build completed unsuccessfully in 0:00:56
[00:03:25] Build completed unsuccessfully in 0:00:56
[00:03:25] Makefile:79: recipe for target 'tidy' failed
[00:03:25] make: *** [tidy] Error 1
The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:00207b3c
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
Sat Dec  1 21:40:56 UTC 2018
---
travis_time:end:04c716dc:start=1543700456757993154,finish=1543700456762634702,duration=4641548
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:26bbde9c
$ ln -s . checkout && for CORE in obj/cores/core.*; do EXE=$(echo $CORE | sed 's|obj/cores/core\.[0-9]*\.!checkout!\(.*\)|\1|;y|!|/|'); if [ -f "$EXE" ]; then printf travis_fold":start:crashlog\n\033[31;1m%s\033[0m\n" "$CORE"; gdb --batch -q -c "$CORE" "$EXE" -iex 'set auto-load off' -iex 'dir src/' -iex 'set sysroot .' -ex bt -ex q; echo travis_fold":"end:crashlog; fi; done || true
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:166ac713
travis_time:start:166ac713
$ cat ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
cat: ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers: No such file or directory
travis_fold:end:after_failure.5
travis_fold:start:after_failure.6
travis_time:start:346f693e
$ dmesg | grep -i kill

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@Zoxc Zoxc force-pushed the Zoxc:arena-tweaks branch from d052736 to a1dade9 Dec 1, 2018

@bors

This comment was marked as outdated.

Contributor

bors commented Dec 2, 2018

☔️ The latest upstream changes (presumably #56378) made this pull request unmergeable. Please resolve the merge conflicts.

@Zoxc Zoxc force-pushed the Zoxc:arena-tweaks branch from a1dade9 to d61be94 Dec 2, 2018

@michaelwoerister

This comment has been minimized.

Contributor

michaelwoerister commented Dec 6, 2018

The perf-results mostly look like noise (except for ctfe-stress). I'm not sure this is worth the more complicated implementation.

cc @rust-lang/wg-compiler-performance

@Zoxc

This comment has been minimized.

Contributor

Zoxc commented Dec 6, 2018

The results doesn't look like noise though. The whole point of the arena is performance. We should just use malloc otherwise =P

@Zoxc

This comment has been minimized.

Contributor

Zoxc commented Dec 6, 2018

This is also a nice size optimization. It brings the fast path for allocating an usize from:

.text:0000000180001235                 mov     rsi, rcx
.text:0000000180001238                 mov     rax, [rcx]
.text:000000018000123B                 mov     rcx, [rcx+8]
.text:000000018000123F                 add     rax, 7
.text:0000000180001243                 and     rax, 0FFFFFFFFFFFFFFF8h
.text:0000000180001247                 mov     [rsi], rax
.text:000000018000124A                 cmp     rcx, rax
.text:000000018000124D                 jb      short loc_180001281
.text:000000018000124F                 mov     rdx, rax
.text:0000000180001252                 add     rdx, 8
.text:0000000180001256                 cmp     rdx, rcx
.text:0000000180001259                 jnb     short loc_18000126B
.text:000000018000125B
.text:000000018000125B loc_18000125B:                          ; CODE XREF: alloctest+4F↓j
.text:000000018000125B                 mov     [rsi], rdx
...
.text:000000018000126B ; ---------------------------------------------------------------------------
.text:000000018000126B
.text:000000018000126B loc_18000126B:                          ; CODE XREF: alloctest+29↑j
.text:000000018000126B                 mov     edx, 8
.text:0000000180001270                 mov     rcx, rsi
.text:0000000180001273                 call    _ZN5arena13DroplessArena4grow17h2cc6dfe0f3889a5eE ; arena::DroplessArena::grow::h2cc6dfe0f3889a5e
.text:0000000180001278                 mov     rax, [rsi]
.text:000000018000127B                 lea     rdx, [rax+8]
.text:000000018000127F                 jmp     short loc_18000125B
.text:0000000180001281 ; ---------------------------------------------------------------------------
.text:0000000180001281
.text:0000000180001281 loc_180001281:                          ; CODE XREF: alloctest+1D↑j
.text:0000000180001281                 lea     rcx, aAssertionFaile ; "assertion failed: self.ptr <= self.end"
.text:0000000180001288                 lea     r8, off_180003230
.text:000000018000128F                 mov     edx, 26h
.text:0000000180001294                 call    _ZN3std9panicking11begin_panic17hca0ac606c2739ec6E ; std::panicking::begin_panic::hca0ac606c2739ec6
.text:0000000180001299 ; ---------------------------------------------------------------------------
.text:0000000180001299                 ud2
.text:0000000180001299 alloctest       endp

to:

.text:00000001800012C4                 mov     rax, [rcx]
.text:00000001800012C7                 lea     rdx, [rax+8]
.text:00000001800012CB                 cmp     rdx, [rcx+8]
.text:00000001800012CF                 jnb     short loc_1800012E0
.text:00000001800012D1                 mov     [rcx], rdx
.text:00000001800012D4
.text:00000001800012D4 loc_1800012D4:                          ; CODE XREF: alloctest+2A↓j
...
.text:00000001800012E0 ; ---------------------------------------------------------------------------
.text:00000001800012E0
.text:00000001800012E0 loc_1800012E0:                          ; CODE XREF: alloctest+F↑j
.text:00000001800012E0                 mov     edx, 8
.text:00000001800012E5                 call    _ZN5arena13DroplessArena18grow_and_alloc_raw17ha43c166c6d43e92fE ; arena::DroplessArena::grow_and_alloc_raw::ha43c166c6d43e92f
.text:00000001800012EA                 jmp     short loc_1800012D4
@michaelwoerister

This comment has been minimized.

Contributor

michaelwoerister commented Dec 7, 2018

I can see the first change making sense but why change from u8 to usize for the drop-less arena?

@Zoxc

This comment has been minimized.

Contributor

Zoxc commented Dec 7, 2018

@michaelwoerister It avoids the instructions realigning the bump pointer by keeping it always usize aligned.

@Zoxc

This comment has been minimized.

Contributor

Zoxc commented Dec 7, 2018

Might make the DroplessArena generic over the backing type, so we could use u8 in the string interner. I think all other allocations are usize aligned.

@michaelwoerister

This comment has been minimized.

Contributor

michaelwoerister commented Dec 7, 2018

I'm still not convinced that there are performance gains here. Compare this to a perf.rlo run from a few days ago where I accidentally benchmarked to equivalent versions of the compiler (so we know what we see is noise):
https://perf.rust-lang.org/compare.html?start=10e2c729ea2e85cb1e2a08be40564492f49e45ec&end=5280d23d48ea46ca2469a77cc240fbef6767efbb

Except for ctfe-stress it looks the same to me.

@nnethercote

This comment has been minimized.

Contributor

nnethercote commented Dec 7, 2018

I agree that the result are probably just noise, with the possible exception of ctfe-stress, but it is one of the most variable benchmarks, which is why it has the '?' markers on its results. So even that could be noise.

@michaelwoerister

This comment has been minimized.

Contributor

michaelwoerister commented Dec 14, 2018

If you remove the BackingType change, I'm happy to approve the PR. I think that always using u8 as the backing type makes the code a lot easier to reason about.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment