Closed Bug 1026545 Opened 10 years ago Closed 6 years ago

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

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED DUPLICATE of bug 1257387

People

(Reporter: mccr8, Unassigned)

References

(Blocks 2 open bugs)

Details

Attachments

(1 file)

- 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().
Oh and also, the callback needs to be registered in CycleCollectedJSRuntime so it is going on both main thread and workers.
Assignee: nobody → continuation
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.
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.
> 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
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.
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).
I'm mostly just worried about how much work it will take to infallibilize certain allocation paths.
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.
Assignee: continuation → nobody
Iain, do you think mccr8's patch might be useful?
Flags: needinfo?(iireland)
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.
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: needinfo?(iireland)
Resolution: --- → DUPLICATE
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: