LeaveTree cannot handle malloc failures

RESOLVED WONTFIX

Status

()

RESOLVED WONTFIX
9 years ago
7 years ago

People

(Reporter: dvander, Assigned: gal)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

+++ This bug was initially created as a clone of Bug #521309 +++

We need to investigate the plausibility and difficulty of LeaveTree propagating an OOM failure. There are cases like JSScope::generateOwnShape and js_GetTopStackFrame, which can deep bail with no clear OOM exit strategy.
From bug 521309 comment 11:

Another idea that would preserve LeaveTree's infallibility is to allocate an emergency reserve for synthesized frames, one such reserve per trace monitor (which is per thread), and use that if we hit OOM, stealing it from the tm and requiring it be replenished. ~300 frames' worth of memory is < 30K.

This seems straightforward to implement. Comments?

Bug 521309 comment 13 said:

I don't think the emergency reserve for frames works though. The only way we allocate is by releasing the arena, so we would have to hook in there to detect when we can release the emergency memory again and we would make the general case slow. But lets discuss all that in a separate bug.

--- here's the separate bug ---

Comment 13 misunderstands the proposal. LeaveTree would use the reserve only if it saw OOM, and claim the reserved space, not leaving it in the TM, which is thread-local. The next ExecuteTree, which is fallible, would replenish the reserve and fail with OOM if it couldn't allocate.

/be
David, you mind taking this? I'll help.

/be
Assignee: general → dvander
Andreas may take this, it sounds like.

/be
(Assignee)

Updated

9 years ago
Blocks: 508140
(Assignee)

Updated

9 years ago
Assignee: dvander → gal
(Assignee)

Comment 4

9 years ago
Created attachment 412070 [details] [diff] [review]
patch

Carry around a 256kb per-thread ballast and release it when malloc fails at a bad time. We use mmap since we can have mmap and malloc failures in LeaveTree.
(Assignee)

Updated

9 years ago
Attachment #412070 - Flags: review?(igor)
(Assignee)

Comment 5

9 years ago
Created attachment 412100 [details] [diff] [review]
patch
Attachment #412070 - Attachment is obsolete: true
Attachment #412070 - Flags: review?(igor)
(Assignee)

Comment 6

9 years ago
Created attachment 412102 [details] [diff] [review]
patch
Attachment #412100 - Attachment is obsolete: true
Attachment #412102 - Flags: review?(igor)
Attachment #412102 - Flags: review?(dvander)
(Assignee)

Comment 7

9 years ago
Created attachment 412106 [details] [diff] [review]
patch
Attachment #412102 - Attachment is obsolete: true
Attachment #412102 - Flags: review?(igor)
Attachment #412102 - Flags: review?(dvander)
Comment on attachment 412106 [details] [diff] [review]
patch

>+        if (chunk == 0) {
>+            if (waiveGCQuota) {
>+                /* We don't want to fail, so try to release some ballast. */
>+                data->releaseBallast();
>+                goto again;
>+            }
>             return NULL;
>+        }

If I understand this correctly, we're assuming the OS will reuse the ballast right away. Is there any way to test this? I think the idea seems right but if we're going to rely on it, we should make sure it works.

Even if that means just cooking up a stupid C++ program that reserves a huge chunk of address space, then a small chunk, then forces a malloc failure and releases the small chunk.
Attachment #412106 - Flags: review+

Comment 9

9 years ago
(In reply to comment #8)
> If I understand this correctly, we're assuming the OS will reuse the ballast
> right away. Is there any way to test this? I think the idea seems right but if
> we're going to rely on it, we should make sure it works.

This is very true. For example, what if the system malloc allocates only, say, 10MB chunks and cannot deal with anything less?

Comment 10

9 years ago
Comment on attachment 412106 [details] [diff] [review]
patch

After the memory is released another thread can immediately consume it.
Attachment #412106 - Flags: review-
These bugs are all part of a search I made for js bugs that are getting lost in transit:

http://tinyurl.com/jsDeadEndBugs

They all have a review+'ed, non-obsoleted patch and are not marked fixed-in-tracemonkey or checkin-needed but have not seen any activity in 300 days. Some of these got lost simply because the assignee/patch provider never requested a checkin, or just because they were forgotten about.
Obsolete with the removal of tracejit.
Status: NEW → RESOLVED
Last Resolved: 7 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.