Closed Bug 1555655 Opened 3 years ago Closed 3 years ago

Crash in [@ OOM | large | Pushing into prim_store.pictures

Categories

(Core :: Graphics: WebRender, defect, P2)

Unspecified
Windows 10
defect

Tracking

()

RESOLVED FIXED
mozilla69
Tracking Status
firefox-esr60 --- unaffected
firefox67 --- wontfix
firefox68 --- wontfix
firefox69 --- fixed

People

(Reporter: nical, Assigned: nical)

References

(Blocks 2 open bugs)

Details

(Keywords: crash)

Crash Data

Attachments

(1 file)

This bug is for crash report bp-9aad39eb-9616-42e2-8a03-8c2e50190524.

Top 10 frames of crashing thread:

0 mozglue.dll mozalloc_abort memory/mozalloc/mozalloc_abort.cpp:33
1 mozglue.dll mozalloc_handle_oom memory/mozalloc/mozalloc_oom.cpp:51
2 xul.dll static void gkrust_shared::oom_hook::hook toolkit/library/rust/shared/lib.rs:255
3 xul.dll static void std::alloc::rust_oom src/libstd/alloc.rs:220
4 xul.dll static void alloc::alloc::handle_alloc_error src/liballoc/alloc.rs:234
5 xul.dll static struct webrender::util::Allocation<webrender::picture::PicturePrimitive> webrender::util::{{impl}}::alloc<webrender::picture::PicturePrimitive> gfx/wr/webrender/src/util.rs:85
6 xul.dll static void webrender::display_list_flattener::DisplayListFlattener::pop_stacking_context gfx/wr/webrender/src/display_list_flattener.rs:1609
7 xul.dll static union core::option::Option<webrender_api::display_list::BuiltDisplayListIter> webrender::display_list_flattener::DisplayListFlattener::flatten_item gfx/wr/webrender/src/display_list_flattener.rs:952
8 xul.dll static void webrender::display_list_flattener::DisplayListFlattener::flatten_items gfx/wr/webrender/src/display_list_flattener.rs:561
9 xul.dll static union core::option::Option<webrender_api::display_list::BuiltDisplayListIter> webrender::display_list_flattener::DisplayListFlattener::flatten_item gfx/wr/webrender/src/display_list_flattener.rs:952

The allocation size (448.8 KB) is surprisingly large.

Summary: Crash in [@ OOM | large | mozalloc_abort | mozalloc_handle_oom | gkrust_shared::oom_hook::hook | std::alloc::rust_oom | webrender::util::{{impl}}::alloc<T>] → Crash in [@ OOM | large | Pushing into prim_store.pictures

It would be great to at least get an idea about what use-cases trigger this, and if any of the WR behavior here is anomalous.

Priority: -- → P2

Would you be able to look at this, Nical?

Flags: needinfo?(nical.bugzilla)

I hadn't realized how enormous the PicturePrimitive struct is (1272 bytes!), which means it only takes about 350 pictures to get to the 448.8 KB (and in practice less since the vector is allocating more than necessary to amortize the cost of future insertions ).

So it doesn't require unreasonable workloads before the vector allocation gets big enough to absorb some of the OOM crashes.

I wouldn't make this a high priority but there are probably low hanging fruits to pick to reduce the size of the structure to win back a bit of memory usage and potentially cache locality (most images in a page are larger).

Flags: needinfo?(nical.bugzilla)

The tile cache is 352 bytes large and in the majority of cases picture primitives don't have one, so this saves a few KB of ram in typical pages reduces the likely hood of hitting OOM crashes while growing the primitives vector.

Another large structure is PictureState (448 bytes) which holds temporary data but it looks like all pictures hold a picture state at some point (between two of the frame building passes). I'm not sure whether we'll be able to remove it eventually?

Flags: needinfo?(gwatson)
Assignee: nobody → nical.bugzilla

I assume most of the pictures created are "pass-through" ones, or otherwise we have bigger issues than OOM :)
Perhaps, we should focus on having pass-through pictures to be light, not carry much of associated data they don't need anyway (e.g. the clip chain instance is guaranteed to be empty).

If PictureState is only used during frame building, then we should have a separate temporary storage for it, owned by the frame building facilities.

Particularly once the new picture caching code lands, we should be able to reduce some of the size of these structures (in particular, the space mappers and some of the other state should become redundant in some of the passes). Probably makes sense to revisit it then.

Flags: needinfo?(gwatson)
Pushed by nsilva@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/eaa4313efee4
Box the TileCache in PicturePrimitive. r=gw
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla69

This doesn't appear to be hitting in the wild very often, so I think we can let this fix ride the trains.

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