521472 - LeaveTree cannot handle malloc failures

Status: RESOLVED WONTFIX

(Core :: JavaScript Engine, defect)

Description

[David Anderson [:dvander] - inactive, e-mail if emergency]

+++ 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.

Comment 1

[Brendan Eich [:brendan]]

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

Comment 2

[Brendan Eich [:brendan]]

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

/be

Comment 3

[Brendan Eich [:brendan]]

Andreas may take this, it sounds like.

/be

Comment 4

[Andreas Gal :gal]

Attachment: 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.

Comment 5

[Andreas Gal :gal]

Attachment: patch

Comment 6

[Andreas Gal :gal]

Attachment: patch

Comment 7

[Andreas Gal :gal]

Attachment: patch

Comment 8

[David Anderson [:dvander] - inactive, e-mail if emergency]

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.

Comment 9

[Igor Bukanov]

(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

[Igor Bukanov]

Comment on attachment 412106 [details] [diff] [review]
patch

After the memory is released another thread can immediately consume it.

Comment 11

[Nochum Sossonko [:Natch]]

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.

Comment 12

[Ryan VanderMeulen [:RyanVM]]

Obsolete with the removal of tracejit.