1026545 - Add a version of js::CrashAtUnhandlableOOM(char const*) that takes a size argument

Status: RESOLVED DUPLICATE of bug 1257387

(Core :: JavaScript Engine, defect)

Description

[Andrew McCreight [:mccr8]]

- Add an OOM callback on the JS runtime that takes a size argument, along with registration hooks.

- In CrashAtUnhandlableOOM() add a size argument with a default value of 0, check if the argument is non-zero.  If so, check if there's an OOM callback registered.  If there is, call the OOM callback.

- In the browser, define an OOM callback that calls |CrashReporter::AnnotateOOMAllocationSize(aSize);|.

- Convert some or all places that call CrashAtUnhandlableOOM to pass in a size value.

I think this should preserve the existing shell behavior of CrashAtUnhandlableOOM().

Comment 1

[Andrew McCreight [:mccr8]]

Oh and also, the callback needs to be registered in CycleCollectedJSRuntime so it is going on both main thread and workers.

Comment 2

[Andrew McCreight [:mccr8]]

Attachment: Add a version of js::CrashAtUnhandlableOOM(char const*) that takes a size argument. WIP

Here's a patch that should implement it, but I think it isn't worth landing until we have some place where we want to actually use it.

As I said in bug 999158, it isn't entirely clear to me what we should report when allocation out of a chunk fails.

Comment 3

[(Away)]

Is it possible to get this landed?

I'm eager for a size annotation, if for no other reason than to distinguish <=1MB failures versus >1MB failures (because I care much more about the latter). How we treat sub-megabyte allocations (whether we report the user-requested size or the allocator-requested size) doesn't matter to me a whole lot. Jemalloc reports the user size so I guess we might as well follow that pattern.

Comment 4

[(Away)]

> Here's a patch that should implement it, but I think it isn't worth landing
> until we have some place where we want to actually use it.

Together these two are half of our "OOM | unknown" or about 3.5% of total OOMs.
http://hg.mozilla.org/releases/mozilla-release/annotate/983a710b51c4/js/src/gc/Nursery.cpp#l577
http://hg.mozilla.org/releases/mozilla-release/annotate/983a710b51c4/js/src/gc/StoreBuffer.h#l139

Comment 5

[Andrew McCreight [:mccr8]]

The problem is that the actual failing allocation (along with its size) is far removed from the location of the crash.

Maybe the easiest thing to do is to set a value of the last failed JS allocation on the runtime somewhere, then we could check it in key places like this when we OOM?  Somebody who knows more about allocation inside the JS engine would probably need to take a look at that.

Once that is done, maybe CrashAtUnhandlableOOM() could just look this up on the runtime, and we wouldn't a version of the function that takes a size argument.

Comment 6

[Emanuel Hoogeveen [:ehoogeveen]]

How much value is there in crashing this high up in the stack? It allows us to supply a reason to CrashAtUnhandlableOOM, but does that give us anything a stack and a comment in the code would not? Is the worry here that we won't have a stack at all, so the reason is the only way we'll be able to find where the allocation came from?

Perhaps SM could use (templated) fallible and infallible allocators - the infallible allocator can just report the size where the failed allocation happens, and anyone calling the fallible allocator should know that it's allocating something big (and deal with it appropriately). IIRC Gecko already does something like this (but the GC has its own allocator).

Comment 7

[Andrew McCreight [:mccr8]]

I'm mostly just worried about how much work it will take to infallibilize certain allocation paths.

Comment 8

[Andrew McCreight [:mccr8]]

Anyways, whatever is done here, it is probably more appropriate to be done by somebody who is more familiar with the JS engine's allocation.

Comment 9

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Iain, do you think mccr8's patch might be useful?

Comment 10

[Iain Ireland [:iain]]

It looks like equivalent code was delivered in bug 1257387.

See here: https://searchfox.org/mozilla-central/source/js/src/vm/JSContext.cpp#1493

Closing as duplicate of that bug.