Update hashbrown and transitive dependencies.

[PiotrSikora]

Signed-off-by: Piotr Sikora piotrsikora@google.com

[gbrail]

This looks fine. However, do we still need hashbrown? Apparently the only reason one needs it today is to work in a "no_std" environment. Can this SDK work in such an environment?

[PiotrSikora]

This looks fine. However, do we still need hashbrown? Apparently the only reason one needs it today is to work in a "no_std" environment. Can this SDK work in such an environment?

While std::HashMap implementation was updated to hashbrown::HashMap in Rust 1.36, it uses HashDoS-resistant SipHash, whereas hashbrown uses not HashDoS-resistant AHash, which is significantly faster.

Regarding no_std, I never tested it, but I suppose it should work.

[codefromthecrypt]

I think we should remove hashbrown as it is a large source of maintenance and contributes to slow compilation. It is odd post 1.36 for us to optimize so specifically. If we did, we should have some benchmarks to show why this is so essential.

[PiotrSikora]

I think we should remove hashbrown as it is a large source of maintenance

According to whom? Based on what? It requires virtually zero maintenance.

and contributes to slow compilation.

Do you have any numbers?

It is odd post 1.36 for us to optimize so specifically. If we did, we should have some benchmarks to show why this is so essential.

There are benchmarks in https://github.com/rust-lang/hashbrown that show that insert and erase are 2x faster, and lookup is 3x faster when using AHash instead of SipHash.

[codefromthecrypt]

https://github.com/proxy-wasm/proxy-wasm-rust-sdk/search?q=hashbrown is all for a single line of code which swaps out the normal map for the hashbrown one

hashbrown plus its dep ahash inside getenvoy's matrix tests take a second to compile * all tests that use it. It is actually hard to measure if there are any other knock-on effects without removing it first (ex resulting size of wasm)

I think benchmarking should be relevant, for example the resulting wasm execution speed if that's really the reason here.

[PiotrSikora]

https://github.com/proxy-wasm/proxy-wasm-rust-sdk/search?q=hashbrown is all for a single line of code which swaps out the normal map for the hashbrown one

Sorry, but what are you trying to prove here? It's a drop-in replacement and it literally couldn't require any less work.

For Cargo builds, you only need to declare hashbrown dependency in Cargo.toml and use it in src/dispatcher.rs.

All other files in your link are autogenerated Bazel rules. Are you suggesting that we remove support for Bazel? It's actually a huge source of maintenance, and most of the commits in this repo are only bumping depenencies in Bazel rules, which we stopped doing recently (See: #99).

hashbrown plus its dep ahash inside getenvoy's matrix tests take a second to compile * all tests that use it. It is actually hard to measure if there are any other knock-on effects without removing it first (ex resulting size of wasm)

If you're looking to fix slow times of Envoy builds/tests, then you're barking at the wrong tree.

I think benchmarking should be relevant, for example the resulting wasm execution speed if that's really the reason here.

I did exactly that in the past, which is why we're using it as a dependency.

[codefromthecrypt]

To step back, I didn't intend to start trouble. I am literally trying to champion use of your copy of proxy-wasm-rust-sdk. There is currently version skew in the fork vs here in hashbrown. I was surprised to see it in use, found someone else asked, so figured I would bring some experience here. That literally go vs rust for wasm, the latter is slow to compile especially on macos and even worse inside a build container. I don't expect to prove that to you, you can accept it or not.

The other thing I ran into was maintenance you rightly put as bazel related, but also a large amount of things just to maintain hashbrown. Indeed that can go away with Bazel, and good riddance if it does.

Core libraries should make speed packs optional not required, as doing so eliminates discussions like this one. I don't expect to change your mind or prove anything, but I did feel like it needed to be said.

Please deep breath on this one.. personally benchmarking a point in time is a "trust me" type of thing. If you really want to champion this it is probably best to put in a target here that benchmarks the resultant wasm. that's more show vs tell, and I don't expect you to do this, but it should be said.

[PiotrSikora]

Don't you see the irony in the fact that you're requesting benchmark targets to prove that hashbrown::HashMap with AHash is faster than the default HashMap (even though those already exist in the official repo), while at the same time claiming some (IMHO, unfounded) issues without any data to back them up, and saying that you "don't expect to prove them"?

But to end this discussion:

1. There is virtually zero maintanence burden because of hashbrown, and even if there was, it's all on me.
2. If using hashbrown::HashMap with AHash instead of the default HashMap results in a slow compilation, then that's unfortunate, but I'll take slow compilation over slow runtime performance every single time (note: the source of slow compilation is probably the inline-more feature in hashbrown).
3. Since hashbrown is used, the onus is you to prove that removing it doesn't degrade the performance.