1631721 - Migrate from hashglobe to hashbrown for Rust fallible heap allocation

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, task)

Description

[Simon Sapin (:SimonSapin)]

Rust APIs are said to have "fallible allocation" when they return an error instead of panicking/aborting when heap memory allocation fails (such as malloc returning null). std::collections::HashMap has a try_reserve method, but it’s not available on the Rust Stable release channel yet: https://github.com/rust-lang/rust/issues/48043

hashglobe is a fork of an old version of that HashMap that allows Stylo to use try_reserve (and adds some more fallible APIs like try_entry).

Since then, the internals of std::collections::HashMap have been entirely replaced by a different implementation which is also available on crates.io as hashbrown. Hashbrown has a try_reserve method available on Rust Stable. This is not a problem since, unlike the standard library, Hashbrown can afford to make breaking changes by picking a next version number that Cargo considers incompatible.

We should consider switching Stylo to use Hashbrown, and then remove Hashglobe:

• This would remove the need to maintain a hash map implementation. Hashbrown is likely better maintained since it is used in the Rust standard library
• The standard library change was motivated by Hashbrown often being faster. Perhaps Stylo’s preformance can benefit as well

Comment 1

[Simon Sapin (:SimonSapin)]

Manish, could you say more about this 'static bound? Why is it needed? What’s the relationship with NonZero? Is it fine to remove it if the compiler is still happy?

https://searchfox.org/mozilla-central/rev/d2cec90777d573585f8477d5170892e5dcdfb0ab/servo/components/style/selector_map.rs#122-124

// FIXME(Manishearth) the 'static bound can be removed when
// our HashMap fork (hashglobe) is able to use NonZero,
// or when stdlib gets fallible collections
impl<T: 'static> SelectorMap<T> {

Comment 2

[Emilio Cobos Álvarez (:emilio)]

(In reply to Simon Sapin (:SimonSapin) from comment #1)

Manish, could you say more about this 'static bound? Why is it needed? What’s the relationship with NonZero? Is it fine to remove it if the compiler is still happy?

https://searchfox.org/mozilla-central/rev/d2cec90777d573585f8477d5170892e5dcdfb0ab/servo/components/style/selector_map.rs#122-124

// FIXME(Manishearth) the 'static bound can be removed when
// our HashMap fork (hashglobe) is able to use NonZero,
// or when stdlib gets fallible collections
impl<T: 'static> SelectorMap<T> {

I'm ~sure that's the case. HashGlobe requires 'static, but if hashbrown doesn't we should be alright.

Comment 3

[Simon Sapin (:SimonSapin)]

Attachment: Bug 1631721 - Vendor the hashbrown crate

This is the hash map implementation now used in the Rust standard library:

• https://blog.rust-lang.org/2019/07/04/Rust-1.36.0.html#a-new-hashmapk-v-implementation
• https://github.com/rust-lang/rust/pull/58623
• https://crates.io/crates/hashbrown

Comment 4

[Simon Sapin (:SimonSapin)]

Attachment: Bug 1631721 - Use hashbrown instead of hashglobe

Comment 5

[Simon Sapin (:SimonSapin)]

Attachment: Bug 1631721 - Use std::alloc instead of hashbrown::alloc in fallible

This works even if the Rust standard library’s allocator is not libc::malloc,
so we can remove the known_system_mallocfeature flag
and make the fallible crate unconditional.

Comment 6

[Simon Sapin (:SimonSapin)]

Attachment: Bug 1631721 - Remove hashglobe

Comment 7

[Manish Goregaokar [:manishearth]]

(In reply to Simon Sapin (:SimonSapin) from comment #1)

Manish, could you say more about this 'static bound? Why is it needed? What’s the relationship with NonZero? Is it fine to remove it if the compiler is still happy?

It's because we used &'static T to emulate NonZero<T>. Totally fine to remove if the compiler is happy.

Comment 8

[Simon Sapin (:SimonSapin)]

https://treeherder.mozilla.org/perf.html#/compare?originalProject=mozilla-central&originalRevision=342710b2598d0c742d2ed782c9bcd73866f1e0be&newProject=try&newRevision=ea68262265abd6851522cee9ced7280e5d9f40aa&framework=1 has both red and green. I’m not sure how to proceed.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/06dcf8ddbfde
Vendor the hashbrown crate r=manishearth
https://hg.mozilla.org/integration/autoland/rev/de10216a9eb7
Use hashbrown instead of hashglobe r=manishearth
https://hg.mozilla.org/integration/autoland/rev/1d992cb96cca
Use std::alloc instead of hashbrown::alloc in fallible r=manishearth
https://hg.mozilla.org/integration/autoland/rev/d0ee684793c4
Remove hashglobe r=manishearth

Comment 10

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/mozilla-central/rev/06dcf8ddbfde
https://hg.mozilla.org/mozilla-central/rev/de10216a9eb7
https://hg.mozilla.org/mozilla-central/rev/1d992cb96cca
https://hg.mozilla.org/mozilla-central/rev/d0ee684793c4

Comment 11

[Alexandru Ionescu (needinfo me) [:alexandrui]]

== Change summary for alert #25703 (as of Fri, 24 Apr 2020 11:03:22 GMT) ==

Improvements:

6% perf_reftest_singletons link-style-cache-1.html linux64-shippable-qr opt e10s stylo 714.43 -> 673.07
6% perf_reftest_singletons link-style-cache-1.html linux64-shippable-qr opt e10s stylo 713.14 -> 673.88
5% perf_reftest_singletons link-style-cache-1.html linux64-shippable opt e10s stylo 690.35 -> 652.55

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=25703

Comment 12

[Florin Strugariu [:Bebe]]

== Change summary for alert #26277 (as of Fri, 19 Jun 2020 05:30:43 GMT) ==

Improvements:

4% Images windows10-64-shippable-qr opt tp6 10,653,356.15 -> 10,241,364.96
4% Images linux1804-64-shippable opt tp6 8,918,652.07 -> 8,575,068.06
3% Images linux1804-64-shippable-qr opt tp6 8,883,869.78 -> 8,598,540.02
3% Images macosx1014-64-shippable opt tp6 9,961,281.38 -> 9,658,324.02

For up to date results, see: https://treeherder.mozilla.org/perf.html#/alerts?id=26277