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

Dogfood new_uninit and maybe_uninit_slice in rustc_arena #76838

Merged
merged 3 commits into from
Sep 19, 2020

Conversation

est31
Copy link
Member

@est31 est31 commented Sep 17, 2020

Dogfoods a few cool MaybeUninit related features in the compiler's rustc_arena crate.

Split off from #76821

r? @oli-obk

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 17, 2020
@est31 est31 force-pushed the dogfood_uninit_features branch 2 times, most recently from 412caf8 to eba9cf2 Compare September 17, 2020 15:27
@est31 est31 force-pushed the dogfood_uninit_features branch from eba9cf2 to 7b99523 Compare September 17, 2020 15:55
@oli-obk
Copy link
Contributor

oli-obk commented Sep 17, 2020

@bors r+

@bors
Copy link
Contributor

bors commented Sep 17, 2020

📌 Commit 7b995232b6a6a02bbd4680e8b3c102623eed966c has been approved by oli-obk

@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 17, 2020
@est31
Copy link
Member Author

est31 commented Sep 18, 2020

Does this need a perf run? I see some regressions in the microbenchmarks, but being microbenchmarks they might lie. Did ./x.py bench --stage 0 compiler/rustc_arena.

Output with base commit from bors:

test tests::bench_copy              ... bench:           5 ns/iter (+/- 0)
test tests::bench_copy_nonarena     ... bench:           0 ns/iter (+/- 0)
test tests::bench_noncopy           ... bench:          64 ns/iter (+/- 1)
test tests::bench_noncopy_nonarena  ... bench:          34 ns/iter (+/- 1)
test tests::bench_typed_arena_clear ... bench:           5 ns/iter (+/- 0)

Output with only the first commit:

test tests::bench_copy              ... bench:           5 ns/iter (+/- 0)
test tests::bench_copy_nonarena     ... bench:           0 ns/iter (+/- 0)
test tests::bench_noncopy           ... bench:          65 ns/iter (+/- 1)
test tests::bench_noncopy_nonarena  ... bench:          36 ns/iter (+/- 1)
test tests::bench_typed_arena_clear ... bench:           7 ns/iter (+/- 0)

Output with both commits:

test tests::bench_copy              ... bench:           5 ns/iter (+/- 0)
test tests::bench_copy_nonarena     ... bench:           0 ns/iter (+/- 0)
test tests::bench_noncopy           ... bench:          63 ns/iter (+/- 1)
test tests::bench_noncopy_nonarena  ... bench:          33 ns/iter (+/- 1)
test tests::bench_typed_arena_clear ... bench:           6 ns/iter (+/- 0

There is tons of noise in the benchmarks but I made multiple runs each time and there is a consistent difference. There might be a regression in bench_typed_arena_clear. Again, it's a microbenchmark and the usage pattern of the arena in the compiler might not reflect it at all.

If I allocate 100 items and do the same clearance I can't find a regression, in fact it's a bit faster now. Benchmark:

#[bench]
pub fn bench_typed_arena_clear_100(b: &mut Bencher) {
    let mut arena = TypedArena::default();
    b.iter(|| {
        for _ in 0..100 {
            arena.alloc(Point { x: 1, y: 2, z: 3 });
        }
        arena.clear();
    })
}

Before this PR:

test tests::bench_typed_arena_clear_100 ... bench:         177 ns/iter (+/- 5)

After this PR:

test tests::bench_typed_arena_clear_100 ... bench:         173 ns/iter (+/- 12)

Again, lot's of noise.

I've added the benchmark to this PR. I've also amended the first commit to do a RawVec → Box change in one of the comments. re-r? @oli-obk

@est31 est31 force-pushed the dogfood_uninit_features branch from 7b99523 to 2805a05 Compare September 18, 2020 04:04
@oli-obk
Copy link
Contributor

oli-obk commented Sep 18, 2020

@bors r+ rollup=never

I don't think this will have an effect on perfbot, but let's not roll this up to make sure

@bors
Copy link
Contributor

bors commented Sep 18, 2020

📌 Commit 2805a05 has been approved by oli-obk

@bors
Copy link
Contributor

bors commented Sep 19, 2020

⌛ Testing commit 2805a05 with merge a2c82df...

@bors
Copy link
Contributor

bors commented Sep 19, 2020

☀️ Test successful - checks-actions, checks-azure
Approved by: oli-obk
Pushing a2c82df to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 19, 2020
@bors bors merged commit a2c82df into rust-lang:master Sep 19, 2020
@rustbot rustbot added this to the 1.48.0 milestone Sep 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. 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.

5 participants