1744102 - Switch to Rust standard library containers for consumers requiring fallible allocation in the style system

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Bobby Holley (:bholley)]

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.

Comment 1

[Paul Adenot (:padenot)]

The audio part is for audioipc.

Comment 2

[Matthew Gregan [:kinetik]]

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.

Comment 3

[Emilio Cobos Álvarez (:emilio)]

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

[Mike Hommey [:glandium]]

At least 2 weeks, to give us time to see whether we're going to stick with it.

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Thanks, filed bug 1744234 to track that.

Comment 6

[Jon Bauman [:jbauman:]]

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.

Comment 7

[Bobby Holley (:bholley)]

(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

[Jon Bauman [:jbauman:]]

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.

Comment 9

[Jon Bauman [:jbauman:]]

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?

Comment 10

[Emilio Cobos Álvarez (:emilio)]

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.

Comment 11

[Emilio Cobos Álvarez (:emilio)]

Attachment: Bug 1744102 - Remove servo/components/{hashglobe,fallible} in favor of try_reserve. r=bholley,#style,#layout-reviewers

Comment 12

[Bobby Holley (:bholley)]

(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

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b050582bc984
Remove servo/components/{hashglobe,fallible} in favor of try_reserve. r=xidorn

Comment 14

[Cristina Cozmuta (:CrissCozmuta)]

https://hg.mozilla.org/mozilla-central/rev/b050582bc984

Comment 15

[Bobby Holley (:bholley)]

Dave, can someone on your team double-check that the regression from bug 1633410 didn't resurface?

Comment 16

[Dave Hunt [:davehunt] [he/him] ⌚GMT]

(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

Comment 17

[Bobby Holley (:bholley)]

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

[Dave Hunt [:davehunt] [he/him] ⌚GMT]

(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.