Switch to Rust standard library containers for consumers requiring fallible allocation in the style system
Categories
(Core :: CSS Parsing and Computation, defect)
Tracking
()
Tracking | Status | |
---|---|---|
firefox97 | --- | fixed |
People
(Reporter: bholley, Assigned: emilio)
References
(Regressed 1 open bug)
Details
Attachments
(1 file)
We have a few consumers in tree that eschew standard Rust containers because of the need for fallible allocation. Stylo was the first one, and since then I believe we've also added usage for AVIF demuxing and some audio stuff.
Rust 1.57 includes built-in fallible allocation, so we should be able to remove this. One wrinkle here is that the standard hashmap algorithm has changed (to swisstable) since we forked it as hashglobe. We tried a while back to switch to the crates.io version of swisstable (hashbrown), and we had to back it out in bug 1633410 due to a memory regression. So we should remeasure and see if it reoccurs.
Reporter | ||
Updated•1 year ago
|
Comment 2•1 year ago
|
||
AudioIPC is using FallibleVec::try_with_capacity in one location. I'll take care of switching it over to Vec::try_reserve_exact and dropping the dependency on fallible_collections.
Assignee | ||
Comment 3•1 year ago
|
||
AIUI even though we build with 1.57 on automation now, minimum version is still 1.53: https://searchfox.org/mozilla-central/rev/3c9369510cb883b9f08d5f9641e1233d2142f025/python/mozboot/mozboot/util.py#13
Mike, can we make 1.57 the minimum version now? Or should we wait? If so, how much? Thanks.
Comment 4•1 year ago
|
||
At least 2 weeks, to give us time to see whether we're going to stick with it.
Assignee | ||
Comment 5•1 year ago
|
||
Thanks, filed bug 1744234 to track that.
Comment 6•1 year ago
|
||
Having done a decent amount of work in this area, I'd recommend caution here. The stabilization of fallible reservation methods is great, but it's still very easy to end up doing infallible allocations since methods like push
will happily allocate infallibly if the reservation logic ever underestimates. There are a lot of situations where allocations happen somewhat non-obviously (for example), so if our goal is to always avoid infallible allocation, I think the distinct types provided by fallible_collections
are still the less error-prone approach.
That said, it would be good to update that crate to use the stdlib implementations, so I filed https://github.com/vcombey/fallible_collections/issues/23.
Reporter | ||
Comment 7•1 year ago
|
||
(In reply to Jon Bauman [:jbauman:] from comment #6)
so if our goal is to always avoid infallible allocation
Well, it's complicated. Allocation in Firefox is infallible by default, and we make allocations fallible on an ad-hoc basis when the callsite starts showing up on crash-stats in significant volume. It's obviously good to anticipate and make fallible potentially large allocations when writing code (so as to avoid the churn of fixing them reactively), but it's not akin to memory safety where we need to get it absolutely right, even for rare corner cases.
That said, if fallible_collections is updated to just be a thin wrapper on top of stdlib, I don't object to folks using it where they feel the extra typechecking is beneficial.
Comment 8•1 year ago
|
||
Point well taken that it's a lower class of risk. That said, I think we prefer to have nice errors rather than tabs crashing.
if fallible_collections is updated to just be a thin wrapper on top of stdlib, I don't object to folks using it where they feel the extra typechecking is beneficial
Sounds good. I'll look into this change as I've formed a positive relationship with the crate author and I don't expect this to be very much work.
Updated•1 year ago
|
Comment 9•1 year ago
|
||
The issue I filed in fallible_collections
has already been fixed, so I suggest we close this issue in favor of bug 1624698. Does that sound good?
Assignee | ||
Comment 10•1 year ago
|
||
I think we probably still want to get rid of hashglobe and such separately. I wrote a patch already so we can use this bug for that.
Assignee | ||
Comment 11•1 year ago
|
||
Updated•1 year ago
|
Reporter | ||
Comment 12•1 year ago
|
||
(In reply to Jon Bauman [:jbauman:] from comment #9)
The issue I filed in
fallible_collections
has already been fixed, so I suggest we close this issue in favor of bug 1624698. Does that sound good?
Because the style system usage is rather involved, I'd like us to first focus on getting back on the stdlib implementation before considering whether we want to use fallible_collections there.
This basically just involves fixing [1] to redirect to the standard lib [2], and then pushing to try to see if the memory regression from bug 1633410 resurfaces.
[1] https://searchfox.org/mozilla-central/rev/fecb1af272051b0fc2346647b1a3193609744384/servo/components/style/hash.rs
[2] Might need a crate-local trait to implement try_entry, since at first glance I don't see that in the stdlib version.
Comment 13•1 year ago
|
||
Pushed by ealvarez@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/b050582bc984 Remove servo/components/{hashglobe,fallible} in favor of try_reserve. r=xidorn
Comment 14•1 year ago
|
||
bugherder |
Reporter | ||
Comment 15•1 year ago
|
||
Dave, can someone on your team double-check that the regression from bug 1633410 didn't resurface?
Comment 16•1 year ago
|
||
(In reply to Bobby Holley (:bholley) from comment #15)
Dave, can someone on your team double-check that the regression from bug 1633410 didn't resurface?
The test results appear to be much noisier recently, and there doesn't appear to be an alert generated this time. I do see an increase when inspecting the Windows results, and have created a manual alert to track this: https://treeherder.mozilla.org/perfherder/alerts?id=32791 though I think the magnitude is closer to 4%.
I see that on resolution of bug 1633410 perf_reftest coalesce-1.html and coalesce-2.html improved. Looking at these, there may be a noticable regression from the time of this change, but no alerts have been generated: https://treeherder.mozilla.org/perfherder/graphs?series=mozilla-central,2819770,1,1&series=autoland,3869360,1,1&timerange=5184000
Reporter | ||
Comment 17•1 year ago
|
||
Thanks. Please retrigger as appropriate to figure out which (if any) regressions are caused by the changeset in this bug, and then we can investigate appropriately.
Comment 18•1 year ago
|
||
(In reply to Bobby Holley (:bholley) from comment #17)
Thanks. Please retrigger as appropriate to figure out which (if any) regressions are caused by the changeset in this bug, and then we can investigate appropriately.
Retriggers and backfills confirm a regression from this changeset. I've opened bug 1747037 for investigation.
Description
•