Closed Bug 1744102 Opened 10 months ago Closed 10 months ago

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

Categories

(Core :: CSS Parsing and Computation, defect)

defect

Tracking

()

RESOLVED FIXED
97 Branch
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.

Flags: needinfo?(emilio)

The audio part is for audioipc.

Flags: needinfo?(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.

Depends on: 1742749
Flags: needinfo?(kinetik)

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.

Flags: needinfo?(emilio) → needinfo?(mh+mozilla)

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

Flags: needinfo?(mh+mozilla)
Depends on: 1744234

Thanks, filed bug 1744234 to track that.

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.

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

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.

Severity: -- → S3

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?

Flags: needinfo?(bholley)

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.

Summary: Switch back to Rust standard library containers for consumers requiring fallible allocation → Switch to Rust standard library containers for consumers requiring fallible allocation in the style system
Assignee: nobody → emilio
Status: NEW → ASSIGNED

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

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
Status: ASSIGNED → RESOLVED
Closed: 10 months ago
Resolution: --- → FIXED
Target Milestone: --- → 97 Branch

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

Flags: needinfo?(bholley) → needinfo?(dave.hunt)

(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

Flags: needinfo?(dave.hunt)

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.

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

Regressions: 1747075
You need to log in before you can comment on or make changes to this bug.