Notify memory-pressure observers when allocating a large ArrayBuffer fails

RESOLVED FIXED in Firefox 28

Status

()

defect
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: mccr8, Assigned: luke)

Tracking

(Blocks 1 bug)

unspecified
mozilla30
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox28 fixed, firefox29 fixed, firefox30 fixed, b2g-v1.3 fixed, b2g-v1.3T fixed, b2g-v1.4 fixed)

Details

(Whiteboard: [MemShrink:P2][games])

Attachments

(5 attachments, 1 obsolete attachment)

Reporter

Description

6 years ago
Using the observer service, you can trigger a memory-pressure event to get the browser to clear the BFcache, dump image buffers, GC/CC, and many other things.  When we fail to allocate a large ArrayBuffer, we could try triggering that.  I'm not sure how synchronous this is, so maybe it won't work well enough?

The code looks like this:
  obsServ->NotifyObservers(nullptr, "memory-pressure", minimize.get());

One thing to note is that I believe this causes us to throw away all WebGL contexts, so this would probably be pretty janky for a game that is already partially loaded.
Assignee

Updated

6 years ago
Whiteboard: [MemShrink] → [MemShrink][games]
Bug 936128 or bug 936218 seem easier.
Whiteboard: [MemShrink][games] → [MemShrink:P3][games]
Assignee

Comment 2

5 years ago
Would the GC/CC/GC happen synchronously (before the NotifyObservers returns)?
Reporter

Comment 3

5 years ago
Yes, you can just call right into the GC and CC.
Assignee

Comment 4

5 years ago
(In reply to Andrew McCreight [:mccr8] from comment #3)
Well, I mean: will calling NotifyObservers(nullptr, "memory-pressure", ...) do this (synchronously)?
Reporter

Comment 5

5 years ago
Yes, I think so.  I'm not sure if we want to always do GC-CC-GC on memory pressure.  But maybe you could add another thing or something.
Assignee

Comment 6

5 years ago
(In reply to Andrew McCreight [:mccr8] from comment #5)
I'm far from the expert on this, but it sounds like, if only a GC-CC-GC can reliably clean up unreachable windows, that that's what you'd want to do.  Or do you mean that we're not always interested in cleaning up garbage so much as clearing caches and the like.
Reporter

Comment 7

5 years ago
Yeah, maybe.  It looks like existing code to do that is in nsJSEnvironmentObserver::Observe.  Maybe it wouldn't hurt to just throw in another GC.  Actually, what we do right now in EndCycleCollectionCallback is check if sCCollectedWaitingForGC > 250 (this means, we have more than 250 things that were freed in the last CC), and if so, then we'd probably have a productive GC, so we do a PokeGC (eg delayed GC).  In the Observe code, we could actually check if that threshold has been exceeded after the forced CC, and if so, run the GC.  That seems reasonable, as we'd be running one again in a few seconds anyways.  (Though if you do that, please make a new constant for that 250...)
Reporter

Updated

5 years ago
Whiteboard: [MemShrink:P3][games] → [MemShrink:P2][games]
Reporter

Comment 8

5 years ago
Well, I'll just file a separate bug on that, and then in this bug you should be able to just trigger the memory pressure event in whatever JS engine place.
Reporter

Updated

5 years ago
Depends on: 965916
Reporter

Updated

5 years ago
No longer depends on: 965916
Assignee

Comment 10

5 years ago
Posted patch notify-on-failure (obsolete) — Splinter Review
Now I just need to make a mochitest for this.
Assignee: nobody → luke
Status: NEW → ASSIGNED
Assignee

Updated

5 years ago
Depends on: 971845
Assignee

Comment 11

5 years ago
I realized, as I wrote the mochitest and had to pick the size, that any non-trivial-sized ArrayBuffer allocation size may end up failing (due to a fragmented long-lived process, which mochitest most definitely is).
Attachment #8375001 - Attachment is obsolete: true
Attachment #8375038 - Flags: review?(continuation)
Assignee

Comment 12

5 years ago
One question: this won't be the first time that the "memory-pressure" event can be triggered while JS is running, right?
Reporter

Comment 13

5 years ago
Comment on attachment 8375038 [details] [diff] [review]
notify-on-failure

Review of attachment 8375038 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good!

FWIW, this isn't going to do anything on workers.  You'll need to register the callback for that on worker threads if you want it to work there.  Assuming the observer service works on workers.  Though I suppose there may be some questions about whether you want to trigger flushing on the main thread vs. other threads...

::: js/src/jsapi.h
@@ +4892,5 @@
>  };
>  
> +
> +/*
> + * If a large allocation fails, the JS engine calls the large-allocation-failure

nit: "calls" -> "may call"?  As only some allocators actually use the callback?

@@ +4894,5 @@
> +
> +/*
> + * If a large allocation fails, the JS engine calls the large-allocation-failure
> + * callback, if set, to allow the embedding to flush caches, possibly perform
> + * shrinking GCs, etc to make some room so that the allocation will succeed if

nit: etc.

::: js/src/vm/Runtime.h
@@ +1791,5 @@
> +    /*
> +     * These variations of malloc/calloc/realloc will call the
> +     * large-allocation-failure callback on OOM and retry the allocation.
> +     */
> +    JS::LargeAllocationFailureCallback largeAllocationFailureCallback;

Do you need to initialize this field in the ctor?

@@ +1795,5 @@
> +    JS::LargeAllocationFailureCallback largeAllocationFailureCallback;
> +
> +    static const unsigned LARGE_ALLOCATION = 25 * 1024 * 1024;
> +
> +    void *reentrantMalloc(size_t bytes) {

"reentrant" seems like a slightly odd name for these, but I suppose "retryable" and "semiinfallible" aren't real words.

::: js/src/vm/TypedArrayObject.cpp
@@ +241,5 @@
>          ObjectElements *oldheader = static_cast<ObjectElements *>(oldptr);
>          uint32_t oldnbytes = ArrayBufferObject::headerInitializedLength(oldheader);
> +        newheader = static_cast<ObjectElements *>(cx->runtime()->reentrantRealloc(oldptr, size));
> +        if (!newheader) {
> +            js_ReportOverRecursed(cx);

Why are you calling ReportOverRecursed here?
Attachment #8375038 - Flags: review?(continuation) → review+
Reporter

Comment 14

5 years ago
(In reply to Luke Wagner [:luke] from comment #12)
> One question: this won't be the first time that the "memory-pressure" event
> can be triggered while JS is running, right?

That's right.  If you go to about:memory and click on "minimize memory usage", I think you are triggering a memory pressure event.
Assignee

Comment 15

5 years ago
(In reply to Luke Wagner [:luke] from comment #12)
Wow, thanks for the careful review.  I wrote the initial version of the patch in a rush and I didn't rereview it carefully enough today.
Assignee

Comment 16

5 years ago
(In reply to Andrew McCreight [:mccr8] from comment #13)
> "reentrant" seems like a slightly odd name for these, but I suppose
> "retryable" and "semiinfallible" aren't real words.

Bill and I like "mallocCanGC"; sound good to you?
Reporter

Comment 17

5 years ago
(In reply to Luke Wagner [:luke] from comment #16)
> Bill and I like "mallocCanGC"; sound good to you?

Sounds much better, thanks!
Assignee

Comment 21

5 years ago
Wow, rooting analysis ftw!  I had forgotten to audit all the callers of AllocateArrayBufferContents for whether they expected GC and indeed, 3 callers were using raw pointers.  Fortunately, all 3 could be easily inlined into their caller which was static and using rooteds, etc.  Patches in a sec.
Assignee

Comment 22

5 years ago
Attachment #8375626 - Flags: review?(jcoppeard)
Assignee

Comment 23

5 years ago
Attachment #8375627 - Flags: review?(jcoppeard)
Assignee

Comment 24

5 years ago
Attachment #8375628 - Flags: review?(jcoppeard)
Comment on attachment 8375626 [details] [diff] [review]
inline-allocate-slots

Review of attachment 8375626 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/public/RootingAPI.h
@@ +850,5 @@
> + * Given a Rooted<JSObject*> obj, one can view
> + *   Handle<StringObject*> h = obj.as<StringObject*>();
> + * as an optimization of
> + *   Rooted<StringObject*> rooted(cx, &obj->as<StringObject*>());
> + *   Handle<StringObject*> h = rooted;

These are not the same if the original rooted obj is modified.

::: js/src/jsobj.h
@@ +1215,5 @@
> +js::RootedBase<JSObject*>::as() const
> +{
> +    const JS::Rooted<JSObject*> &self = *static_cast<const JS::Rooted<JSObject*>*>(this);
> +    JS_ASSERT(self->is<U>());
> +    return Handle<U*>::fromMarkedLocation(reinterpret_cast<U* const*>(self.address()));

Can we get away with static_cast here?
Attachment #8375626 - Flags: review?(jcoppeard) → review+
Attachment #8375627 - Flags: review?(jcoppeard) → review+
Attachment #8375628 - Flags: review?(jcoppeard) → review+
Assignee

Comment 28

5 years ago
I realized that we should use onOutOfMemory after the largeAllocationFailure callback is called so that we'll wait for any background sweeping/freeing to complete.
Attachment #8376366 - Flags: review?(jcoppeard)
Comment on attachment 8376366 [details] [diff] [review]
fix-malloc-can-gc

Review of attachment 8376366 [details] [diff] [review]:
-----------------------------------------------------------------

Yes, looks good.
Attachment #8376366 - Flags: review?(jcoppeard) → review+
Depends on: 1088249
You need to log in before you can comment on or make changes to this bug.