1000632 - OdinMonkey: further optimize the Odin to Ion FFI calls.

Status: RESOLVED DUPLICATE of bug 1288483

(Core :: JavaScript Engine: JIT, defect, P5)

Description

[Douglas Crosher [:dougc]]

Bug 898963 comment 38 confirms that setActive can be simplified.  This simplifies the Odin to Ion FFI call instruction sequences.

Comment 1

[Douglas Crosher [:dougc]]

Attachment: Simply setActive.

Here's a patch to try an implement the setActive simplification.  Unfortunately it has not been a success and crashes, e.g. asm.js/testBullet.js

Comment 2

[Hannes Verschore [:h4writer]]

Attachment: Extra

You will also need this. Since if we go this route, we need to initialize everything upfront.

Comment 3

[Douglas Crosher [:dougc]]

Attachment: Partly simplify setActive.

A little blind hacking gives this as a reduced setActive that avoids crashing the jit-tests.  Keeping the IonTop related code seem necessary to avoid a crash.  Need to explore the real issue.

Comment 4

[Douglas Crosher [:dougc]]

Attachment: Partly simplified setActive.

Rebase.

Comment 5

[Luke Wagner [:luke]]

I might understand the failure here.  The invariant we must preserve is that, at all times in asm.js code and when initially calling Ion from asm.js is: ionTop == ionTop on entry to asm.js.  However, asm.js calls Ion and Ion returns to asm.js, ionTop may have been clobbered by calls to C++ from Ion code.  Normally ~JitActivation makes this ok but we're explicitly skipping that.

If this is correct, then it suggests that we do need the ionTop = prevIonTop on the "Disable Activation" path, but we can omit the prevIonTop = ionTop store on the "Enable Activation" path.

Comment 6

[Benjamin Bouvier [:bbouvier] (inactive)]

This is something we might still want to have; though I wonder if it would be superseded by work to make ion->wasm calls faster too, since that would merge the JitActivation with the WasmActivation, iirc.

Comment 7

[Luke Wagner [:luke]]

Actually, these optimizations ended up going into 75fbd8a85688 as part of bug 1288483 recently.