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

Do not grow the container if an insertion is on a tombstone. #75

Closed
wants to merge 1 commit into from
Closed

Do not grow the container if an insertion is on a tombstone. #75

wants to merge 1 commit into from

Conversation

alkis
Copy link
Contributor

@alkis alkis commented Apr 26, 2019

No description provided.

@Amanieu
Copy link
Member

Amanieu commented Apr 26, 2019

Can you provide benchmark results for this change?

@alkis
Copy link
Contributor Author

alkis commented Apr 26, 2019

Changes are in the noise. Unsurprisingly since the benchmarks don't test insert/erase workloads. The insert/erase inserts a bunch and removes a bunch so the difference in the code doesn't matter. It does verify it does not make them slower.

Before:

test insert_erase_i32 ... bench:      24,370 ns/iter (+/- 4,587)
test insert_erase_i64 ... bench:      24,403 ns/iter (+/- 1,900)
test insert_i32       ... bench:      19,143 ns/iter (+/- 2,207)
test insert_i64       ... bench:      20,110 ns/iter (+/- 1,531)
test iter_i32         ... bench:       1,643 ns/iter (+/- 304)
test iter_i64         ... bench:       1,640 ns/iter (+/- 54)
test lookup_fail_i32  ... bench:       1,759 ns/iter (+/- 562)
test lookup_fail_i64  ... bench:       1,912 ns/iter (+/- 57)
test lookup_i32       ... bench:       2,447 ns/iter (+/- 174)
test lookup_i64       ... bench:       2,358 ns/iter (+/- 74)

After

test insert_erase_i32 ... bench:      24,310 ns/iter (+/- 1,222)
test insert_erase_i64 ... bench:      25,604 ns/iter (+/- 15,702)
test insert_i32       ... bench:      19,896 ns/iter (+/- 13,620)
test insert_i64       ... bench:      19,917 ns/iter (+/- 816)
test iter_i32         ... bench:       1,655 ns/iter (+/- 131)
test iter_i64         ... bench:       1,649 ns/iter (+/- 153)
test lookup_fail_i32  ... bench:       1,749 ns/iter (+/- 104)
test lookup_fail_i64  ... bench:       1,995 ns/iter (+/- 796)
test lookup_i32       ... bench:       2,439 ns/iter (+/- 126)
test lookup_i64       ... bench:       2,404 ns/iter (+/- 650)

Any idea why insert_no_grow is now detected as dead even if it is used by rustc_entry.rs? Ideas to fix this?

@alkis
Copy link
Contributor Author

alkis commented Apr 26, 2019

Perhaps we can run some kind of real-life benchmark? Rustc maybe? Any instructions on how to do that?

@Amanieu
Copy link
Member

Amanieu commented Apr 27, 2019

You can try using https://github.com/rust-lang-nursery/rustc-perf to see the impact of the change on a wide range of crates (including rustc). You will need to change src/libstd/Cargo.toml to point to your modified hashbrown instead of the one from crates.io, and then build a custom rustc toolchain using that libstd.

@Amanieu
Copy link
Member

Amanieu commented Jul 3, 2019

CI is failing because insert_no_grow is an unused function. Can you mark it with #[cfg(feature = "rustc-dep-of-std")] since it is only used by rustc_entry?

@Amanieu
Copy link
Member

Amanieu commented Jul 29, 2019

Closing in favor of #106.

@Amanieu Amanieu closed this Jul 29, 2019
bors added a commit that referenced this pull request Jul 29, 2019
 Do not grow the container if an insertion is on a tombstone.

Rebased and fixed version of #75.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants