462042 - TM: Allow GC with traced machine code on stack

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jason Orendorff [:jorendorff]]

If we allow reentering the VM as discussed in bug 462027, we need to cope with stacks like

js_Interpret -> js_ExecuteTree -> (traced code) -> Array_p_join -> js_ValueToString -> js_Invoke -> js_Interpret -> ... -> js_GC

Currently this will do js_GC -> js_FlushJITCache -> Fragmento::clearFrags, which would free the traced code.  We will eventually return to that code, so we need to keep it around, or something.

Comment 1

[Benjamin Smedberg]

Can you just suppress GC while on trace?

Comment 2

[Brendan Eich [:brendan]]

We do suppress GC on trace. Trick here is GC'ing after reconstructing stack frames and otherwise making sure everything created on trace is rooted, and unwinding carefully.

We also need to stop flushing the JIT cache from the GC. David recently added a generation number to the code cache, which paves the way for not flushing the JIT cache from the GC.

/be

Comment 3

[Benjamin Smedberg]

Jason and I were talking, and I wondered if you could flush the JIT code during GC: then simply bypass it when walking back up the stack by returning/jumping back to js_ExecuteTree (using assembly) and skipping the native code entirely.

Comment 4

[Benjamin Smedberg]

Bug 458288 fixed this in the simple case: we don't flush the trace cache at GC, we wait until it unwinds and starts tracing again. However, what will happen in the following case?:

js_Interpret -> js_ExecuteTree -> (traced code) -> Array_p_join ->
js_ValueToString -> js_Invoke -> js_Interpret -> ... -> js_GC

Continue in js_Interpret... will tracing (and perhaps JIT) resume immediately? if so, won't that destroy the outer traced code?

Comment 5

[Brendan Eich [:brendan]]

(In reply to comment #4)
> Bug 458288 fixed this in the simple case: we don't flush the trace cache at GC,
> we wait until it unwinds and starts tracing again. However, what will happen in
> the following case?:
> 
> js_Interpret -> js_ExecuteTree -> (traced code) -> Array_p_join ->
> js_ValueToString -> js_Invoke -> js_Interpret -> ... -> js_GC
> 
> Continue in js_Interpret...

You mean the second js_Interpret -> ... (I'm pretty sure).

> will tracing (and perhaps JIT) resume immediately?

Yes.

> if so, won't that destroy the outer traced code?

Yes, that's a hole in the current scheme (danderson says).

Nevertheless we probably want to avoid longjmp or equivalent just to get out of GC hell.

Also, js_GC does bails out early with JS_ON_TRACE(cx). Is there a problem here for sure? We might have memory-pressure-induced OOMs, at worst.

/be

Comment 6

[Jason Orendorff [:jorendorff]]

(In reply to comment #5)
> Also, js_GC does bails out early with JS_ON_TRACE(cx). Is there a problem here
> for sure? We might have memory-pressure-induced OOMs, at worst.

Hmm. I filed this because I wanted to change that behavior: instead of refusing to GC and staying on trace, bail off trace and proceed with GC. I'll try it without and see how bad it is.

Comment 7

[Andreas Gal :gal]

as long we have a reserve pool to guarantee that we can bail out we should be fine

Comment 8

[Jason Orendorff [:jorendorff]]

Attachment: v1

This patch applies on top of the one in bug 462027.  The problem is introduced in that patch.

The fix is yet another flag.  We could instead go to a status enum.

Comment 9

[Brendan Eich [:brendan]]

JSPackedBools all in a row if not an enum. What are the valid state combos?

/be

Comment 10

[Jason Orendorff [:jorendorff]]

Valid state combos are listed in the bug; I'll switch to JSPackedBools.  It seems like someone cleaned out JSTraceMonitor, so the words "yet another" no longer apply.

Comment 11

[Jason Orendorff [:jorendorff]]

Sorry, "listed in a comment in the patch", not "in the bug".

Comment 12

[Jason Orendorff [:jorendorff]]

Attachment: v2

Updated with JSPackedBool.

Comment 13

[Brendan Eich [:brendan]]

Comment on attachment 356264 [details] [diff] [review]
v2

Looks good, thanks.

/be

Comment 14

[Jason Orendorff [:jorendorff]]

Attachment: v3 - updated

The same as v2 but updated to apply cleanly over the much-revised patch in bug 462027.  Carrying over brendan's review.

Comment 15

[Jason Orendorff [:jorendorff]]

Attachment: v4

Rebased, and fixed a silly bug introduced in v3 by my failure to review hg's handiwork when it says "hunk 32 succeeded at offset 1799 lines with fuzz 2".

Carrying forward r+ again.

Comment 16

[Jason Orendorff [:jorendorff]]

Attachment: v5 - updated

And rebased to tip once again.

Comment 17

[Jason Orendorff [:jorendorff]]

Attachment: v6 - unbitrotted again

Comment 18

[Jason Orendorff [:jorendorff]]

http://hg.mozilla.org/tracemonkey/rev/2b3e64603943

Comment 19

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/2b3e64603943

Comment 20

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/9e8b7a72b6b7