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

Status: RESOLVED FIXED

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

Description

[Nicolas Silva [:nical]]

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

Comment 1

[Nicolas Silva [:nical]]

The allocation size (448.8 KB) is surprisingly large.

Comment 2

[Dzmitry Malyshau [:kvark]]

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.

Comment 3

[Jessie [:jbonisteel] pls NI]

Would you be able to look at this, Nical?

Comment 4

[Nicolas Silva [:nical]]

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

Comment 5

[Nicolas Silva [:nical]]

Attachment: Bug 1555655 - Box the TileCache in PicturePrimitive. r=gw

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.

Comment 6

[Nicolas Silva [:nical]]

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?

Comment 7

[Dzmitry Malyshau [:kvark]]

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

Comment 8

[Dzmitry Malyshau [:kvark]]

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

Comment 9

[Glenn Watson [:gw] | PTO until 29th April]

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.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/eaa4313efee4
Box the TileCache in PicturePrimitive. r=gw

Comment 11

[Raul Gurzau (:RaulG)]

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

Comment 12

[Ryan VanderMeulen [:RyanVM]]

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