Closed Bug 722238 Opened 12 years ago Closed 12 years ago

IonMonkey: implement simpler mechanism for On Stack Invalidation

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: cdleary, Assigned: cdleary)

References

Details

Attachments

(1 file, 3 obsolete files)

Attached patch OSI the Next Generation (obsolete) — Splinter Review
One of the ideas we were kicking around early on for OSI was to SMC-patch the return point (for an invalidate-able call) into a jump to the invalidation machinery. Turns out that this approach is significantly simpler than the patch-the-return-address mechanism implemented in bug 686927 and dvander says it's cleaner enough that we should go for it!

The main trouble is that invalidate-able calls (calls that may perform GC or reflow type information) need a snapshot from *after* the call occurs. This snapshot lets us reify interpreter state if the LIR instruction causes the script performing the call to become invalidated. Note that you can't replay an effect-ful call, so the interpter must start at a PC that *follows* the LIR instruction such that it won't be executed twice. (Note that the call was successfully performed, the caller just "happened" to be invalidated as a result.)

The issue with a "post-call" snapshot is that the register allocator (RA) operates solely at the level of LIR instructions -- the RA state at a call that's performed somewhere in the middle of the codegen for a LIR instruction is not easily captured nor expressed.

As a solution, we capture the "post-call" state with a successor, dummy LIR instruction, called LOsiPoint. [*]_ An LOsiPoint is generated as a successor to any LIR instruction that requests a safepoint. [*]_ When that LIR becomes invalidated, we permit it to run to the beginning of the LOsiPoint, through any move groups that RA may have inserted between the LIR instruction and the LOsiPoint, to the snapshot captured for OSI. This works as the natural flow for both the inline and out-of-line paths codegen'd from a LIR instruction.

.. [*] Used to be LCaptureAllocations.
.. [*] Safepoints are necessary for potentially-GCing or effectful calls created during code generation.

I got the attachment working for recursive-invalidate on x86 on a plane ride today. It still needs to be updated to ensure that there's enough patchable space between any two given LOsiPoints (and made to work on the other architectures), but I think the mechanism is basically in place.
Attached patch WIP: OSI-NG. (obsolete) — Splinter Review
This patch is looking good on my debug tests on x86.
Attachment #592628 - Attachment is obsolete: true
Actually, looks like it reduces the x86 jit-test failures to just this:

FAILURES:
    --ion -n /moz/ion-osi/js/src/jit-test/tests/basic/splice-check-steps.js
    --ion -n /moz/ion-osi/js/src/jit-test/tests/basic/testArrayDensityChange.js
    --ion -n /moz/ion-osi/js/src/jit-test/tests/jaeger/getter-hook-2.js
    --ion -n /moz/ion-osi/js/src/jit-test/tests/sunspider/check-3d-cube.js
TIMEOUTS:
    --ion -n /moz/ion-osi/js/src/jit-test/tests/v8-v5/check-crypto.js

So 11 drops to 5, and there are only 3 failures now (down from ~10) on x64:

FAILURES:
    --ion -n /moz/ion-osi/js/src/jit-test/tests/basic/splice-check-steps.js
    --ion -n /moz/ion-osi/js/src/jit-test/tests/jaeger/getter-hook-2.js
    --ion -n /moz/ion-osi/js/src/jit-test/tests/v8-v5/check-crypto.js

I'll clean the patch up so we can get this thing reviewed and landed.
I realized there's one thing missing: we need to be able to decref the IonScripts when we're popping invalidated frames due to an exception. We could do this by checking to see if the OSI point corresponding to the call safepoint contains a call to the invalidation epilogue.

(This approach means that we'll have to patch precisely the OSI sites that were invalidated, instead of a conservative approach of patching all of the ones in the script like we were pondering offline.)
(In reply to Chris Leary [:cdleary] from comment #3)
> I realized there's one thing missing: we need to be able to decref the
> IonScripts when we're popping invalidated frames due to an exception. We
> could do this by checking to see if the OSI point corresponding to the call
> safepoint contains a call to the invalidation epilogue.

Hrm - right. Given that we want script->ion to be NULL immediately after invalidation (to not delay recompilation), options are kind of limited, and it is tricky to recover the IonScript inside of HandleExceptions. I can't see any way to do it with just the (calleeToken, fd, returnAddress) tuple.

Some cheap tricks come to mind but they all involve allocation in some way which sucks, and since we really want this to be infallible we'd have to also implement a fallback. Maybe we should just suck up the cost and leave an empty slot in every function frame for invalidation use.

> (This approach means that we'll have to patch precisely the OSI sites that
> were invalidated, instead of a conservative approach of patching all of the
> ones in the script like we were pondering offline.)

I think you can still patch all of them.
One of the cheap tricks would be to make IonScript a singly linked list, and adding an invalidatedScripts field to JSScript. If HandleException sees script->ion is NULL or returnAddress is not in script->ion->code's range, we then search the linked list for the right script.

Both it and the bailout code would have to take care to decref and remove from the list as necessary. But everything would be allocation-free.
Attached patch OSI-NG (obsolete) — Splinter Review
Except for exception handling I think this is good to go. We should discuss the pros/cons of the approaches in person tomorrow and I'll make an additional patch for that.

The way of solving it I mention in the IonFrameIterator::jsFrameInvalidated comment isn't fallible / doesn't use additional memory, but you might think it's too complex to warrant the savings -- I'm not sure myself. :-)
Attachment #592983 - Attachment is obsolete: true
Attachment #593032 - Flags: review?(dvander)
(In reply to David Anderson [:dvander] from comment #5)
> One of the cheap tricks would be to make IonScript a singly linked list, and
> adding an invalidatedScripts field to JSScript. If HandleException sees
> script->ion is NULL or returnAddress is not in script->ion->code's range, we
> then search the linked list for the right script.
> 
> Both it and the bailout code would have to take care to decref and remove
> from the list as necessary. But everything would be allocation-free.

Could you make the linked list hang off the runtime or IonCompartment?  JM+TI does something like this to keep track of stack frames which were in the middle of a native call or getter hook when they were invalidated, in order to make sure the pools associated with those stubs get freed (it can't directly patch the return address for native/getter calls).
David and I chatted about it offline and we came up with a hybrid of my crazy code-patching approach and his return-address-in-range tactic that won't use any more resources. Writing that patch now.
Blocks: 721031
Attached patch OSI-NGSplinter Review
Now accounts for exception-handling frame-popping.
Attachment #593032 - Attachment is obsolete: true
Attachment #593032 - Flags: review?(dvander)
Attachment #593264 - Flags: review?(dvander)
Comment on attachment 593032 [details] [diff] [review]
OSI-NG

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

::: js/src/ion/Bailouts.cpp
@@ +331,5 @@
> +    //      returnAddress
> +    //
> +    // So we have to munge the frame descriptor size up a word to
> +    // account for the calleeToken that's not normally present in exit
> +    // frames.

Maybe a clearer explanation is:
  * The frame descriptor contains the size of the caller's locals, but not the caller or callee's frame headers. When we remove the bailed frame and link it as an exit frame, the pushed callee token is no longer part of any frame header, and thus we must change the caller's frame descriptor to include it as a local. Otherwise, stack traversal code will fail because it is off by one word.

::: js/src/ion/IonBuilder.cpp
@@ +3348,5 @@
>          }
>      }
>  
>      TypeOracle::Binary binary = oracle->binaryOp(script, pc);
> +    (void) binary; // FIXME

Can we just remove the call to binaryOp?

::: js/src/ion/shared/Assembler-x86-shared.h
@@ +854,5 @@
>      }
>  };
>  
> +static inline size_t
> +PatchWrite_NearCallSize()

Could all of these functions be static members of Assembler?
Attachment #593032 - Flags: review+
Comment on attachment 593264 [details] [diff] [review]
OSI-NG

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

Nice work. I would run a simple known-OSI test through valgrind just to make sure we're not leaking IonScripts.

::: js/src/ion/IonCode.h
@@ +164,5 @@
>  
> +    // Offset of the invalidation epilogue (which pushes this IonScript
> +    // and calls the invalidation thunk).
> +    uint32 invalidateEpilogueOffset_;
> +    // The offset immediately after the IonScript immediate.

nit: Newline above this comment.

::: js/src/ion/IonFrameIterator.h
@@ +93,5 @@
>      }
> +    // Returns whether the JS frame has been invalidated and, if so,
> +    // places the invalidated Ion script in |ionScript|.
> +    bool jsFrameInvalidated(IonScript **ionScript) const;
> +    bool jsFrameInvalidated() const;

Would another name work here? Like "checkInvalidation"?

::: js/src/ion/IonFrames.cpp
@@ +174,5 @@
> +
> +    int32 invalidationDataOffset = ((int32 *) returnAddr)[-1];
> +    uint8 *ionScriptDataOffset = returnAddr + invalidationDataOffset;
> +    IonScript *ionScript = (IonScript *) ((uintptr_t *) ionScriptDataOffset)[-1];
> +    JS_ASSERT(ionScript->containsCodeAddress(returnAddr));

Nice!

::: js/src/jit-test/jit_test.py
@@ +501,5 @@
>          ion_flags = [ 
>                        ['--ion', '-n'],
> +                      #['--ion', '-n', '--ion-regalloc=greedy'],
> +                      #['--ion', '-n', '--ion-gvn=off', '--ion-licm=off' ],
> +                      #['--ion', '-n', '--ion-gvn=off', '--ion-licm=off', '--ion-regalloc=greedy']

Make sure not to include this.
Attachment #593264 - Flags: review?(dvander) → review+
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.