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

Replace HashMap implementation with SwissTable #56241

Closed
wants to merge 20 commits into from

Conversation

@Amanieu
Copy link
Contributor

Amanieu commented Nov 26, 2018

The implementation is from the hashbrown crate.

This is mostly complete, however it is missing 2 features:

  • Adaptive early resizing. -- doesn't seem to be needed
  • try_reserve -- DONE

cc @pietroalbini @Gankro

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Nov 26, 2018

r? @sfackler

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

@pietroalbini

This comment has been minimized.

Copy link
Member

pietroalbini commented Nov 26, 2018

@bors try

cc @rust-lang/infra, this should get a perf run as soon as the try build finishes (it's needed for the second day of impl days at rustfest rome)

bors added a commit that referenced this pull request Nov 26, 2018

Auto merge of #56241 - Amanieu:hashbrown2, r=<try>
WIP: Replace HashMap implementation with SwissTable

The implementation is from the [hashbrown](https://github.com/Amanieu/hashbrown) crate.

This is mostly complete, however it is missing 2 features:
- Adaptive early resizing.
- `try_reserve`

cc @pietroalbini @Gankro
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Nov 26, 2018

⌛️ Trying commit cfd3225 with merge 5aaebb9...

@rust-highfive

This comment was marked as outdated.

Copy link
Collaborator

rust-highfive commented Nov 26, 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:1529e080:start=1543249255992989136,finish=1543249321851190115,duration=65858200979
$ 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:36]    Compiling panic_unwind v0.0.0 (/checkout/src/libpanic_unwind)
[00:03:40] warning: unused variable: `additional`
[00:03:40]    --> src/libstd/collections/hash/map.rs:412:35
[00:03:40]     |
[00:03:40] 412 |     pub fn try_reserve(&mut self, additional: usize) -> Result<(), CollectionAllocErr> {
[00:03:40]     |                                   ^^^^^^^^^^ help: consider using `_additional` instead
[00:03:40]     = note: #[warn(unused_variables)] on by default
[00:03:40] 
[00:03:52]     Finished release [optimized] target(s) in 45.76s
[00:03:52] Copying stage0 std from stage0 (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu / x86_64-unknown-linux-gnu)
---
[00:18:31]    Compiling core v0.0.0 (/checkout/src/libcore)
[00:18:31]    Compiling unwind v0.0.0 (/checkout/src/libunwind)
[00:18:31]    Compiling build_helper v0.1.0 (/checkout/src/build_helper)
[00:18:36] error[E0432]: unresolved import
[00:18:36]   --> src/libcore/num/dec2flt/rawfp.rs:33:11
[00:18:36]    |
[00:18:36] 33 | use fmt::{Debug, LowerExp};
[00:18:36] 
[00:18:36] error: cannot determine resolution for the derive macro `Debug`
[00:18:36] error: cannot determine resolution for the derive macro `Debug`
[00:18:36]   --> src/libcore/num/dec2flt/rawfp.rs:40:23
[00:18:36]    |
[00:18:36] 40 | #[derive(Copy, Clone, Debug)]
[00:18:36]    |
[00:18:36]    = note: import resolution is stuck, try simplifying macro imports
[00:18:36] 
[00:18:36] 
[00:18:37] error[E0416]: identifier `Acquire` is bound more than once in the same pattern
[00:18:37]      |
[00:18:37]      |
[00:18:37] 2091 |         (Acquire, Acquire) => intrinsics::atomic_cxchg_acq(dst, old, new),
[00:18:37]      |                   ^^^^^^^ used in a pattern more than once
[00:18:37] 
[00:18:37] error[E0416]: identifier `Relaxed` is bound more than once in the same pattern
[00:18:37]      |
[00:18:37]      |
[00:18:37] 2094 |         (Relaxed, Relaxed) => intrinsics::atomic_cxchg_relaxed(dst, old, new),
[00:18:37]      |                   ^^^^^^^ used in a pattern more than once
[00:18:37] 
[00:18:37] error[E0416]: identifier `SeqCst` is bound more than once in the same pattern
[00:18:37]      |
[00:18:37]      |
[00:18:37] 2095 |         (SeqCst, SeqCst) => intrinsics::atomic_cxchg(dst, old, new),
[00:18:37]      |                  ^^^^^^ used in a pattern more than once
[00:18:37] 
[00:18:37] error[E0416]: identifier `Acquire` is bound more than once in the same pattern
[00:18:37]      |
[00:18:37]      |
[00:18:37] 2116 |         (Acquire, Acquire) => intrinsics::atomic_cxchgweak_acq(dst, old, new),
[00:18:37]      |                   ^^^^^^^ used in a pattern more than once
[00:18:37] 
[00:18:37] error[E0416]: identifier `Relaxed` is bound more than once in the same pattern
[00:18:37]      |
[00:18:37]      |
[00:18:37] 2119 |         (Relaxed, Relaxed) => intrinsics::atomic_cxchgweak_relaxed(dst, old, new),
[00:18:37]      |                   ^^^^^^^ used in a pattern more than once
[00:18:37] 
[00:18:37] error[E0416]: identifier `SeqCst` is bound more than once in the same pattern
[00:18:37]      |
[00:18:37]      |
[00:18:37] 2120 |         (SeqCst, SeqCst) => intrinsics::atomic_cxchgweak(dst, old, new),
[00:18:37]      |                  ^^^^^^ used in a pattern more than once
[00:18:37] error: aborting due to 8 previous errors
[00:18:37] 
[00:18:37] Some errors occurred: E0416, E0432.
[00:18:37] For more information about an error, try `rustc --explain E0416`.
[00:18:37] For more information about an error, try `rustc --explain E0416`.
[00:18:37] error: Could not compile `core`.
[00:18:37] warning: build failed, waiting for other jobs to finish...
[00:18:38] error: build failed
[00:18:38] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "build" "--target" "x86_64-unknown-linux-gnu" "-j" "4" "--release" "--locked" "--color" "always" "--features" "panic-unwind backtrace" "--manifest-path" "/checkout/src/libstd/Cargo.toml" "--message-format" "json"
[00:18:38] expected success, got: exit code: 101
[00:18:38] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap build
[00:18:38] Build completed unsuccessfully in 0:15:32
[00:18:38] Makefile:28: recipe for target 'all' failed
[00:18:38] make: *** [all] Error 1
The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 2.
travis_time:start:04a839fb
$ date && (curl -fs --head https://google.com | grep ^Date: | sed 's/Date: //g' || true)
Mon Nov 26 16:40:49 UTC 2018

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)

@pietroalbini

This comment has been minimized.

Copy link
Member

pietroalbini commented Nov 26, 2018

@bors try delegate+

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Nov 26, 2018

✌️ @Amanieu can now approve this pull request

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Nov 26, 2018

⌛️ Trying commit 03db6f4 with merge 73919a1...

bors added a commit that referenced this pull request Nov 26, 2018

Auto merge of #56241 - Amanieu:hashbrown2, r=<try>
WIP: Replace HashMap implementation with SwissTable

The implementation is from the [hashbrown](https://github.com/Amanieu/hashbrown) crate.

This is mostly complete, however it is missing 2 features:
- Adaptive early resizing.
- `try_reserve`

cc @pietroalbini @Gankro
@rust-highfive

This comment was marked as outdated.

Copy link
Collaborator

rust-highfive commented Nov 26, 2018

The job dist-x86_64-linux 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_fold:end:services

travis_fold:start:git.checkout
travis_time:start:1f8f5510
$ git clone --depth=2 --branch=try https://github.com/rust-lang/rust.git rust-lang/rust
---
[00:05:22]    Compiling panic_unwind v0.0.0 (/checkout/src/libpanic_unwind)
[00:05:26] warning: unused variable: `additional`
[00:05:26]    --> src/libstd/collections/hash/map.rs:412:35
[00:05:26]     |
[00:05:26] 412 |     pub fn try_reserve(&mut self, additional: usize) -> Result<(), CollectionAllocErr> {
[00:05:26]     |                                   ^^^^^^^^^^ help: consider using `_additional` instead
[00:05:26]     = note: #[warn(unused_variables)] on by default
[00:05:26] 
[00:05:40]     Finished release [optimized] target(s) in 50.48s
[00:05:40] Copying stage0 std from stage0 (x86_64-unknown-linux-gnu -> x86_64-unknown-linux-gnu / x86_64-unknown-linux-gnu)
---
[00:49:56]    Compiling core v0.0.0 (/checkout/src/libcore)
[00:49:56]    Compiling unwind v0.0.0 (/checkout/src/libunwind)
[00:49:56]    Compiling build_helper v0.1.0 (/checkout/src/build_helper)
[00:50:00] error[E0432]: unresolved import
[00:50:00]   --> src/libcore/num/dec2flt/rawfp.rs:33:11
[00:50:00]    |
[00:50:00] 33 | use fmt::{Debug, LowerExp};
[00:50:00] 
[00:50:00] error: cannot determine resolution for the derive macro `Debug`
[00:50:00] error: cannot determine resolution for the derive macro `Debug`
[00:50:00]   --> src/libcore/num/dec2flt/rawfp.rs:40:23
[00:50:00]    |
[00:50:00] 40 | #[derive(Copy, Clone, Debug)]
[00:50:00]    |
[00:50:00]    = note: import resolution is stuck, try simplifying macro imports
[00:50:00] 
[00:50:00] 
[00:50:01] error[E0416]: identifier `Acquire` is bound more than once in the same pattern
[00:50:01]      |
[00:50:01]      |
[00:50:01] 2091 |         (Acquire, Acquire) => intrinsics::atomic_cxchg_acq(dst, old, new),
[00:50:01]      |                   ^^^^^^^ used in a pattern more than once
[00:50:01] 
[00:50:01] error[E0416]: identifier `Relaxed` is bound more than once in the same pattern
[00:50:01]      |
[00:50:01]      |
[00:50:01] 2094 |         (Relaxed, Relaxed) => intrinsics::atomic_cxchg_relaxed(dst, old, new),
[00:50:01]      |                   ^^^^^^^ used in a pattern more than once
[00:50:01] 
[00:50:01] error[E0416]: identifier `SeqCst` is bound more than once in the same pattern
[00:50:01]      |
[00:50:01]      |
[00:50:01] 2095 |         (SeqCst, SeqCst) => intrinsics::atomic_cxchg(dst, old, new),
[00:50:01]      |                  ^^^^^^ used in a pattern more than once
[00:50:01] 
[00:50:01] error[E0416]: identifier `Acquire` is bound more than once in the same pattern
[00:50:01]      |
[00:50:01]      |
[00:50:01] 2116 |         (Acquire, Acquire) => intrinsics::atomic_cxchgweak_acq(dst, old, new),
[00:50:01]      |                   ^^^^^^^ used in a pattern more than once
[00:50:01] 
[00:50:01] error[E0416]: identifier `Relaxed` is bound more than once in the same pattern
[00:50:01]      |
[00:50:01]      |
[00:50:01] 2119 |         (Relaxed, Relaxed) => intrinsics::atomic_cxchgweak_relaxed(dst, old, new),
[00:50:01]      |                   ^^^^^^^ used in a pattern more than once
[00:50:01] 
[00:50:01] error[E0416]: identifier `SeqCst` is bound more than once in the same pattern
[00:50:01]      |
[00:50:01]      |
[00:50:01] 2120 |         (SeqCst, SeqCst) => intrinsics::atomic_cxchgweak(dst, old, new),
[00:50:01]      |                  ^^^^^^ used in a pattern more than once
[00:50:02] error: aborting due to 8 previous errors
[00:50:02] 
[00:50:02] Some errors occurred: E0416, E0432.
[00:50:02] For more information about an error, try `rustc --explain E0416`.
---
travis_time:end:0d9aa0f5:start=1543252380560909682,finish=1543252380570961872,duration=10052190
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:009923ec
$ 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:2056231c
travis_time:start:2056231c
$ 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:14508654
$ 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)

@rust-highfive

This comment was marked as outdated.

Copy link
Collaborator

rust-highfive commented Nov 26, 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:03b58678:start=1543251899547419502,finish=1543251953974008641,duration=54426589139
$ 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:19:12]    Compiling core v0.0.0 (/checkout/src/libcore)
[00:19:12]    Compiling build_helper v0.1.0 (/checkout/src/build_helper)
[00:19:12]    Compiling unwind v0.0.0 (/checkout/src/libunwind)
[00:19:17] error[E0432]: unresolved import
[00:19:17]   --> src/libcore/num/dec2flt/rawfp.rs:33:11
[00:19:17]    |
[00:19:17] 33 | use fmt::{Debug, LowerExp};
[00:19:17] 
[00:19:17] error: cannot determine resolution for the derive macro `Debug`
[00:19:17] error: cannot determine resolution for the derive macro `Debug`
[00:19:17]   --> src/libcore/num/dec2flt/rawfp.rs:40:23
[00:19:17]    |
[00:19:17] 40 | #[derive(Copy, Clone, Debug)]
[00:19:17]    |
[00:19:17]    = note: import resolution is stuck, try simplifying macro imports
[00:19:17] 
[00:19:17] 
[00:19:17] error[E0416]: identifier `Acquire` is bound more than once in the same pattern
[00:19:17]      |
[00:19:17]      |
[00:19:17] 2091 |         (Acquire, Acquire) => intrinsics::atomic_cxchg_acq(dst, old, new),
[00:19:17]      |                   ^^^^^^^ used in a pattern more than once
[00:19:17] 
[00:19:17] error[E0416]: identifier `Relaxed` is bound more than once in the same pattern
[00:19:17]      |
[00:19:17]      |
[00:19:17] 2094 |         (Relaxed, Relaxed) => intrinsics::atomic_cxchg_relaxed(dst, old, new),
[00:19:17]      |                   ^^^^^^^ used in a pattern more than once
[00:19:17] 
[00:19:17] error[E0416]: identifier `SeqCst` is bound more than once in the same pattern
[00:19:17]      |
[00:19:17]      |
[00:19:17] 2095 |         (SeqCst, SeqCst) => intrinsics::atomic_cxchg(dst, old, new),
[00:19:17]      |                  ^^^^^^ used in a pattern more than once
[00:19:17] 
[00:19:17] error[E0416]: identifier `Acquire` is bound more than once in the same pattern
[00:19:17]      |
[00:19:17]      |
[00:19:17] 2116 |         (Acquire, Acquire) => intrinsics::atomic_cxchgweak_acq(dst, old, new),
[00:19:17]      |                   ^^^^^^^ used in a pattern more than once
[00:19:17] 
[00:19:17] error[E0416]: identifier `Relaxed` is bound more than once in the same pattern
[00:19:17]      |
[00:19:17]      |
[00:19:17] 2119 |         (Relaxed, Relaxed) => intrinsics::atomic_cxchgweak_relaxed(dst, old, new),
[00:19:17]      |                   ^^^^^^^ used in a pattern more than once
[00:19:17] 
[00:19:17] error[E0416]: identifier `SeqCst` is bound more than once in the same pattern
[00:19:17]      |
[00:19:17] 2120error: aborting due to 8 previous errors
[00:19:19] 
[00:19:19] Some errors occurred: E0416, E0432.
---
[00:19:19] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "build" "--target" "x86_64-unknown-linux-gnu" "-j" "4" "--release" "--locked" "--color" "always" "--features" "panic-unwind backtrace" "--manifest-path" "/checkout/src/libstd/Cargo.toml" "--message-format" "json"
[00:19:19] expected success, got: exit code: 101
[00:19:19] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap build
[00:19:19] Build completed unsuccessfully in 0:15:50
[00:19:19] Makefile:28: recipe for target 'all' failed
[00:19:19] make: *** [all] Error 1
57304 ./obj/build/x86_64-unknown-linux-gnu/stage0-sysroot/lib/rustlib
57300 ./obj/build/x86_64-unknown-linux-gnu/stage0-sysroot/lib/rustlib/x86_64-unknown-linux-gnu
57296 ./obj/build/x86_64-unknown-linux-gnu/stage0-sysroot/lib/rustlib/x86_64-unknown-linux-gnu/lib
56896 ./src/llvm/test/MC

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.

Copy link
Contributor

bors commented Nov 26, 2018

💔 Test failed - status-travis

@rust-highfive

This comment was marked as outdated.

Copy link
Collaborator

rust-highfive commented Nov 26, 2018

The job dist-x86_64-linux 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_fold:end:services

travis_fold:start:git.checkout
travis_time:start:05edbc7a
$ git clone --depth=2 --branch=try https://github.com/rust-lang/rust.git rust-lang/rust
---
[00:47:07]    Compiling core v0.0.0 (/checkout/src/libcore)
[00:47:07]    Compiling build_helper v0.1.0 (/checkout/src/build_helper)
[00:47:07]    Compiling unwind v0.0.0 (/checkout/src/libunwind)
[00:47:10] error[E0432]: unresolved import
[00:47:10]   --> src/libcore/num/dec2flt/rawfp.rs:33:11
[00:47:10]    |
[00:47:10] 33 | use fmt::{Debug, LowerExp};
[00:47:10] 
[00:47:10] error: cannot determine resolution for the derive macro `Debug`
[00:47:10] error: cannot determine resolution for the derive macro `Debug`
[00:47:10]   --> src/libcore/num/dec2flt/rawfp.rs:40:23
[00:47:10]    |
[00:47:10] 40 | #[derive(Copy, Clone, Debug)]
[00:47:10]    |
[00:47:10]    = note: import resolution is stuck, try simplifying macro imports
[00:47:10] 
[00:47:10] 
[00:47:10] error[E0416]: identifier `Acquire` is bound more than once in the same pattern
[00:47:10]      |
[00:47:10]      |
[00:47:10] 2091 |         (Acquire, Acquire) => intrinsics::atomic_cxchg_acq(dst, old, new),
[00:47:10]      |                   ^^^^^^^ used in a pattern more than once
[00:47:10] 
[00:47:10] error[E0416]: identifier `Relaxed` is bound more than once in the same pattern
[00:47:10]      |
[00:47:10]      |
[00:47:10] 2094 |         (Relaxed, Relaxed) => intrinsics::atomic_cxchg_relaxed(dst, old, new),
[00:47:10]      |                   ^^^^^^^ used in a pattern more than once
[00:47:10] 
[00:47:10] error[E0416]: identifier `SeqCst` is bound more than once in the same pattern
[00:47:10]      |
[00:47:10]      |
[00:47:10] 2095 |         (SeqCst, SeqCst) => intrinsics::atomic_cxchg(dst, old, new),
[00:47:10]      |                  ^^^^^^ used in a pattern more than once
[00:47:10] 
[00:47:10] error[E0416]: identifier `Acquire` is bound more than once in the same pattern
[00:47:10]      |
[00:47:10]      |
[00:47:10] 2116 |         (Acquire, Acquire) => intrinsics::atomic_cxchgweak_acq(dst, old, new),
[00:47:10]      |                   ^^^^^^^ used in a pattern more than once
[00:47:10] 
[00:47:10] error[E0416]: identifier `Relaxed` is bound more than once in the same pattern
[00:47:10]      |
[00:47:10]      |
[00:47:10] 2119 |         (Relaxed, Relaxed) => intrinsics::atomic_cxchgweak_relaxed(dst, old, new),
[00:47:10]      |                   ^^^^^^^ used in a pattern more than once
[00:47:10] 
[00:47:10] error[E0416]: identifier `SeqCst` is bound more than once in the same pattern
[00:47:10]      |
[00:47:10]      |
[00:47:10] 2120 |         (SeqCst, SeqCst) => intrinsics::atomic_cxchgweak(dst, old, new),
[00:47:10]      |                  ^^^^^^ used in a pattern more than once
[00:47:11] error: aborting due to 8 previous errors
[00:47:11] 
[00:47:11] Some errors occurred: E0416, E0432.
[00:47:11] For more information about an error, try `rustc --explain E0416`.
---
travis_time:end:0a9aeb56:start=1543254768289710638,finish=1543254768295832212,duration=6121574
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:056e60b6
$ 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:1c0c9ede
travis_time:start:1c0c9ede
$ 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:171ff89d
$ 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)

@Amanieu

This comment has been minimized.

Copy link
Contributor Author

Amanieu commented Nov 27, 2018

It seems that the new HashMap is causing errors in name resolution. However I am not familiar with the code. Could someone point me at the HashMaps that are likely to cause the errors above?

cc @petrochenkov

@petrochenkov

This comment has been minimized.

Copy link
Contributor

petrochenkov commented Nov 27, 2018

@Amanieu
use fmt::Debug blocks resolution of #[derive(Debug)].
Apparently we can't determine that it doesn't import anything named Debug in macro namespace soon enough.

The workaround is to remove use fmt::Debug; and use fmt::Debug in code.

@petrochenkov

This comment has been minimized.

Copy link
Contributor

petrochenkov commented Nov 27, 2018

Oh, wait a second, this PR doesn't actually modify rawfp.rs and libcore in general.
That's worse.

@petrochenkov

This comment has been minimized.

Copy link
Contributor

petrochenkov commented Nov 27, 2018

Did the previous HashMap have any guarantees about "order of elements == order of insertion", or something like that, accidentally?

@Amanieu

This comment has been minimized.

Copy link
Contributor Author

Amanieu commented Nov 27, 2018

@petrochenkov There are no ordering guarantees in HashMap, so it is very likely that the ordering has changed when the HashMap implementation changed. Is there anything in name resolution that relies on the insertion order?

I am currently trying to narrow down the exact hash map that is causing the issue. I can confirm that switching only librustc_resolve to the new hashmap reproduces this issue (everything else still uses the old hashmap).

Note that it is also possible that there is a bug in the new HashMap.

@petrochenkov

This comment has been minimized.

Copy link
Contributor

petrochenkov commented Nov 27, 2018

There are no ordering guarantees in HashMap, so it is very likely that the ordering has changed

Fix: order of elements with a hash collision, not the general order.
But that probably doesn't matter because our hash maps cannot be used as multi-maps.

I am currently trying to narrow down the exact hash map that is causing the issue. I can confirm that switching only librustc_resolve to the new hashmap reproduces this issue (everything else still uses the old hashmap).

Yes, it's better to bisect all uses of FxHashMap in rustc_resolve and find out what exact map is responsible, then it will be clearer what happens on the resolve side.

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Dec 23, 2018

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

@Gankro

This comment has been minimized.

Copy link
Contributor

Gankro commented Dec 23, 2018

(my review is currently on hold as i deal with some medical issues, sorry)

@stokhos

This comment has been minimized.

Copy link

stokhos commented Jan 14, 2019

Ping from triage @sfackler have you had time to review this pr?

@Amanieu

This comment has been minimized.

Copy link
Contributor Author

Amanieu commented Jan 14, 2019

@alexcrichton suggested that instead of copying hashbrown into libstd, we could have libstd import hashbrown directly as an extern crate. libstd would then just have an API shim with documentation and stability attributes that wraps the actual implementation in the hashbrown crate.

@TimNN

This comment has been minimized.

Copy link
Contributor

TimNN commented Jan 22, 2019

Ping from triage @Amanieu / @alexcrichton: What are the plans for this PR?

@Gankro

This comment has been minimized.

Copy link
Contributor

Gankro commented Jan 22, 2019

(still on medical leave)

i do not think we should extern hashbrown without the review we would require to merge this code

@gnzlbg

This comment has been minimized.

Copy link
Contributor

gnzlbg commented Jan 23, 2019

I would prefer to:

  • wait for @Gankro's review before changing libstd to use hashbrown,
  • in the mean time,
    • change this PR to add hashbrown as an an external crate since that would allow us to set up some more extensive CI for it, instead of adding that to the CI budget of libstd (e.g. do some fuzzing, etc.). This would mean that @Gankro 's review would need to continue in the hashbrown repo though

    • do a crater run once this PR is green (have we done that already?). I think that for this PR is ok to do a couple of crater runs. One crater run now might uncover issues, and we should definitely do a crater run right before merging after all review comments have been addressed to make sure that none of the fixes broke anything.

@Gankro get well soon and don't stress yourself about the review !

@Amanieu

This comment has been minimized.

Copy link
Contributor Author

Amanieu commented Jan 23, 2019

I'm going to leave the PR as it is until the review is finished. I will be integrating any feedback back into the hashbrown repo.

@panaman67

This comment has been minimized.

Copy link

panaman67 commented Jan 24, 2019

Really excited for this commit. Great work!

@TimNN TimNN added S-blocked and removed S-waiting-on-author labels Jan 29, 2019

@TimNN

This comment has been minimized.

Copy link
Contributor

TimNN commented Jan 29, 2019

(still on medical leave)

i do not think we should extern hashbrown without the review we would require to merge this code

Thanks for the update!

@Gankro get well soon and don't stress yourself about the review !

Couldn't agree more, get better soon!


I'm marking this as blocked for the moment, so it doesn't show up during the regular PR triage.

@alkis
Copy link
Contributor

alkis left a comment

It is exciting to see SwissTable making it into the Rust standard library! Thanks @Amanieu for doing all the work.

}
fn make_hash<K: Hash + ?Sized>(hash_builder: &impl BuildHasher, val: &K) -> u64 {
let mut state = hash_builder.build_hasher();
val.hash(&mut state);

This comment has been minimized.

@alkis

alkis Feb 4, 2019

Contributor

One potential mitigation to the quadradic insertion bug is to always hash the pointer to the buckets as well as the key. This will completely eliminate the quadradic behavior on the merge_dos benchmark but it will slow down resizing the buckets array because it will effectively reshuffle all elements on every resize and this will have worse cache performance than the current approach.

This comment has been minimized.

@Amanieu

Amanieu Feb 5, 2019

Author Contributor

I considered that but I don't feel comfortable mixing address bits into the hash key. I think that this could allow an attacker to defeat address randomization by inferring address bits from looking at the iteration order of a hash table.

// Gotta resize now.
// Ideally we would put this in VacantEntry::insert, but Entry is not
// generic over the BuildHasher and adding a generic parameter would be
// a breaking change.

This comment has been minimized.

@alkis

alkis Feb 4, 2019

Contributor

Why can't you move the reserve(1) inside the else block?

This comment has been minimized.

@Amanieu

Amanieu Feb 4, 2019

Author Contributor

Because the reserve could invalidate the result of the lookup if it rehashes the table.

This comment has been minimized.

@alkis

alkis Feb 4, 2019

Contributor

It is ok if it does. In most cases there is no need to rehash. If a rehash happens, the cost of an additional probe round is insignificant. Right now this reserve(1) is breaking the API guarantees. reserve(N) followed by N entry(K).insert(V) will cause a rehash even though it is specified that it will not.

Edit: also the way this is coded you can defer reserve(1) until the entry is inserted, right?

This comment has been minimized.

@Amanieu

Amanieu Feb 5, 2019

Author Contributor

Sorry, you were right, this reserve call should indeed be in the else block.

We can't defer the reserve call until the entry is inserted because Entry would need to hold a reference to the hasher, which is not possible since in libstd Entry is Entry<K, V> instead of Entry<K, V, S> and changing that would be a breaking change.

key: K,
elem: VacantEntryState<K, V, &'a mut RawTable<K, V>>,
table: &'a mut RawTable<(K, V)>,

This comment has been minimized.

@alkis

alkis Feb 4, 2019

Contributor

To get the VacantEntry we already searched the table and we know exactly in which Bucket we will insert. I think this struct needs to hold a Bucket otherwise insert() will need to probe again.

This comment has been minimized.

@Amanieu

Amanieu Feb 4, 2019

Author Contributor

There are different constraints when searching for an element vs searching for an insertion slot. In particular, we ignore (skip over) deleted buckets in the former case while in the latter case we can insert on top of a deleted bucket.

This comment has been minimized.

@alkis

alkis Feb 4, 2019

Contributor

Apologies for the confusing comment. I think the Entry API allows two implementations here. On return:

  1. VacantEntry already points to the actual bucket and insert() is trivial.
  2. VacantEntry caches the key and hash and insert() does the actual insert/grow dance.

In either case the optimal way to implement find_or_insert semantics needed by entry() -> insert() is:

  • find probe, if found return it (insert() won't be called)
  • insert probe, if bucket is deleted or if growth_left > 0 return it (this probe costs very little because we are touching at most the same cachelines we touched in the previous probe - a deleted bucket can only make us return earlier)
  • rehash and do another insert probe to find where to insert it (the cost of an additional probe here is insignificant - we already did N probes to rehash the container and copied N key, value tuples)
Show resolved Hide resolved src/libstd/collections/hash/raw/mod.rs
Show resolved Hide resolved src/libstd/collections/hash/raw/mod.rs
Show resolved Hide resolved src/libstd/collections/hash/raw/mod.rs
Show resolved Hide resolved src/libstd/collections/hash/raw/mod.rs
// This may panic.
let hash = hasher(item.as_ref());

// We can use a simpler version of insert() here since there are no

This comment has been minimized.

@alkis

alkis Feb 4, 2019

Contributor

The reason find_insert_slot() can be used is because 1) we know there is enough space for all entries and 2) we know all items are unique.

Show resolved Hide resolved src/libstd/collections/hash/raw/mod.rs
@Amanieu

This comment has been minimized.

Copy link
Contributor Author

Amanieu commented Feb 21, 2019

I opened a new PR (#58623) which makes libstd depend on hashbrown from crates.io.

bors added a commit that referenced this pull request Feb 25, 2019

Auto merge of #58623 - Amanieu:hashbrown3, r=<try>
Replace HashMap implementation with SwissTable (as an external crate)

This is the same as #56241 except that it imports `hashbrown` as an external crate instead of copying the implementation into libstd.

This includes a few API changes (all unstable):
- `try_reserve` is added to `HashSet`.
- Some trait bounds have been changed in the `raw_entry` API.
- `search_bucket` has been removed from the `raw_entry` API (doesn't work with SwissTable).
@Gankro
Copy link
Contributor

Gankro left a comment

I am back from leave and am resuming the review. This batch represents my review of RawTable, which contains a few serious correctness concerns.

#[inline]
pub unsafe fn drop(&self) {
self.ptr.as_ptr().drop_in_place();
}

This comment has been minimized.

@Gankro

Gankro Mar 4, 2019

Contributor

Unless you need some kind of interior mutability, you should make the mutating methods take &mut self just so &SomethingContainingABucket can't accidentally do interior mutability things. Also just makes it clear to the reader that there isn't anything weird going on here.

}

// Branch prediction hint. This is currently only available on nightly but it
// consistently improves performance by 10-15%.

This comment has been minimized.

@Gankro

Gankro Mar 4, 2019

Contributor

reminder that this is a thing (status change?)

Show resolved Hide resolved src/libstd/collections/hash/raw/mod.rs
Show resolved Hide resolved src/libstd/collections/hash/raw/mod.rs
Show resolved Hide resolved src/libstd/collections/hash/raw/mod.rs
Show resolved Hide resolved src/libstd/collections/hash/raw/mod.rs
Show resolved Hide resolved src/libstd/collections/hash/raw/mod.rs
Show resolved Hide resolved src/libstd/collections/hash/raw/mod.rs
Show resolved Hide resolved src/libstd/collections/hash/raw/mod.rs
Show resolved Hide resolved src/libstd/collections/hash/raw/mod.rs

Amanieu added a commit to Amanieu/hashbrown that referenced this pull request Mar 5, 2019

@Amanieu

This comment has been minimized.

Copy link
Contributor Author

Amanieu commented Mar 5, 2019

I have addressed most of the feedback in this commit: Amanieu/hashbrown@5d1d4e9

Note that I'm not updating this PR any more, all future development towards integrating hashbrown into libstd is in these two PRs:

@Gankro

This comment has been minimized.

Copy link
Contributor

Gankro commented Mar 6, 2019

Since this has gotten pretty out-of-sync with the actual code we should probably drop this review, and I'll just do an "offline" review of map.rs (since I expect it to be pretty straightforward, esp. since you moved most logic into the raw_table).

Also I'm not sure how important handling size_of<(K, V)>() == 0 particularly gracefully can matter? There's no way to feed in state for hashing to be anything other than random, right? Like sure we shouldn't do UB, but otherwise I think we can spin our wheels or maybe even just panic?

@Amanieu

This comment has been minimized.

Copy link
Contributor Author

Amanieu commented Mar 6, 2019

The handling of ZSTs should be fixed now (see my changes to Bucket).

I'm going to close this PR, any further reviews should be on one of the two PRs linked above.

@Amanieu Amanieu closed this Mar 6, 2019

Amanieu added a commit to Amanieu/hashbrown that referenced this pull request Mar 6, 2019

Amanieu added a commit to Amanieu/hashbrown that referenced this pull request Mar 8, 2019

Amanieu added a commit to Amanieu/hashbrown that referenced this pull request Mar 26, 2019

Amanieu added a commit to Amanieu/hashbrown that referenced this pull request Mar 29, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.