Closed Bug 1000632 Opened 6 years ago Closed 3 years ago

OdinMonkey: further optimize the Odin to Ion FFI calls.

Categories

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

defect

Tracking

()

RESOLVED DUPLICATE of bug 1288483

People

(Reporter: dougc, Unassigned)

Details

Attachments

(3 files, 1 obsolete file)

Bug 898963 comment 38 confirms that setActive can be simplified.  This simplifies the Odin to Ion FFI call instruction sequences.
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
Attached patch ExtraSplinter Review
You will also need this. Since if we go this route, we need to initialize everything upfront.
Attached patch Partly simplify setActive. (obsolete) — Splinter Review
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.
Rebase.
Attachment #8411837 - Attachment is obsolete: true
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.
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.
Priority: -- → P5
Actually, these optimizations ended up going into 75fbd8a85688 as part of bug 1288483 recently.
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 1288483
You need to log in before you can comment on or make changes to this bug.