Closed Bug 1640942 Opened 5 years ago Closed 4 years ago

Improve Rust OOM signatures

Categories

(Socorro :: Signature, task, P2)

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: mccr8, Assigned: willkg)

References

Details

Attachments

(1 file)

Two recent bugs, bug 1640564 and bug 1639996, have shown that we have pretty unpleasant signature for Rust OOMs.

[@ OOM | large | mozalloc_abort | mozalloc_handle_oom | gkrust_shared::oom_hook::hook | std::alloc::rust_oom | hashbrown::raw::RawTable<T>::new_uninitialized<T>]

[@ OOM | large | mozalloc_abort | <name omitted> | gkrust_shared::oom_hook::hook | rust_oom]

mozalloc_handle_oom, gkrust_shared::oom_hook::hook, std::alloc::rust_oom, and rust_oom all look like boilerplate that should be added to the skip list. I guess most of these are in the prefix list already, because they are getting the frame past them, but it doesn't seem that useful.

mozalloc_abort might also be nice to remove, but I'm not sure where else it might show up.

RawTable<T>::new_uninitialized should probably also be added to the prefix list, so we can split signatures depending on the places that are creating the hash tables.

I don't really have time at the moment to dig into this issue further, but I wanted to get a bug filed for this.

rust_oom isn't in the prefix list which is why that second signature ends there.

Seems like we should make the following changes:

  1. add hashbrown::raw::RawTable<T>::new_uninitialized<T> to the prefix list so signature generation continues

  2. then either add rust_oom to the prefix list so signature generation continues,

    OR add mozalloc_handle_oom, gkrust_shared::oom_hook::hook, std::alloc::rust_oom, and rust_oom to the irrelevant list.

Andrew: Does that summarize what you're suggesting?

Gabriele: Does this make sense to do?

Flags: needinfo?(gsvelto)
Flags: needinfo?(continuation)

Makes sense. We've got a lot of OOM-related functions in the prefix list so I wonder if the irrelevant list option is the best one. Having a super-long signature is not really helpful especially in an OOM crash that is usually already clearly marked as such.

Flags: needinfo?(gsvelto)

Yes, that matches what I'm thinking of. mozalloc_abort could probably go too, though I do see some signatures like [@ mozalloc_abort | sk_abort_no_print | SkGlyphRunListPainter::drawForBitmapDevice ] where this is really just a MOZ_CRASH, so maybe we can leave it alone. Just one junky-ish frame isn't too bad.

Flags: needinfo?(continuation)

Makes sense--thank you!

Grabbing this to do soon.

Assignee: nobody → willkg
Status: NEW → ASSIGNED
Priority: -- → P2

Thanks!

Deployed this to prod in bug #1642634. Marking as FIXED.

Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: