Skip to content

Dogfood new_uninit and maybe_uninit_slice in rustc_arena#76838

Merged
bors merged 3 commits intorust-lang:masterfrom
est31:dogfood_uninit_features
Sep 19, 2020
Merged

Dogfood new_uninit and maybe_uninit_slice in rustc_arena#76838
bors merged 3 commits intorust-lang:masterfrom
est31:dogfood_uninit_features

Conversation

@est31
Copy link
Member

@est31 est31 commented Sep 17, 2020 •
edited
Loading

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
oli-obk
oli-obk reviewed Sep 17, 2020
View reviewed changes
@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
Collaborator

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
Collaborator

bors commented Sep 18, 2020

📌 Commit 2805a05 has been approved by oli-obk

@bors
Copy link
Collaborator

bors commented Sep 19, 2020

⌛ Testing commit 2805a05 with merge a2c82df...

@bors
Copy link
Collaborator

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

Reviewers

@oli-obk oli-obk oli-obk left review comments

Assignees

@oli-obk oli-obk

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

Milestone

1.48.0

Development

Successfully merging this pull request may close these issues.

5 participants

@est31 @oli-obk @bors @rust-highfive @rustbot