Last Comment Bug 860838 - OdinMonkey: optimize asm.js FFI calls
: OdinMonkey: optimize asm.js FFI calls
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla24
Assigned To: Hannes Verschore [:h4writer]
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on: 864468 882399 882843 882974 883211 883490 883626 884989 886266 886287 886966 944278
Blocks: 896808
  Show dependency treegraph
 
Reported: 2013-04-11 11:20 PDT by Luke Wagner [:luke]
Modified: 2013-11-29 15:26 PST (History)
8 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
WIP (24.17 KB, patch)
2013-05-03 08:31 PDT, Hannes Verschore [:h4writer]
no flags Details | Diff | Splinter Review
WIP 2.0 (28.06 KB, patch)
2013-05-07 10:45 PDT, Hannes Verschore [:h4writer]
no flags Details | Diff | Splinter Review
WIP 3.0 (37.01 KB, patch)
2013-05-08 08:25 PDT, Hannes Verschore [:h4writer]
no flags Details | Diff | Splinter Review
Patch (38.10 KB, patch)
2013-05-13 04:40 PDT, Hannes Verschore [:h4writer]
no flags Details | Diff | Splinter Review
Revert bug 871016 (1.81 KB, patch)
2013-05-18 10:10 PDT, Hannes Verschore [:h4writer]
luke: review+
Details | Diff | Splinter Review
patch b (improvements + fix crash) (18.09 KB, patch)
2013-06-06 05:58 PDT, Hannes Verschore [:h4writer]
no flags Details | Diff | Splinter Review
Folded patch applies on trunk (43.12 KB, patch)
2013-06-07 17:08 PDT, Hannes Verschore [:h4writer]
no flags Details | Diff | Splinter Review
Fixes for browser/arm + comments addressed (30.73 KB, patch)
2013-06-11 06:29 PDT, Hannes Verschore [:h4writer]
no flags Details | Diff | Splinter Review
Combined patch (48.37 KB, patch)
2013-06-11 08:56 PDT, Hannes Verschore [:h4writer]
luke: review+
Details | Diff | Splinter Review
Fix for the stack changes overnight (7.38 KB, patch)
2013-06-12 07:26 PDT, Hannes Verschore [:h4writer]
luke: review+
Details | Diff | Splinter Review

Description Luke Wagner [:luke] 2013-04-11 11:20:16 PDT
FFI calls (when asm.js calls non-asm.js) currently just call a generic stub that boxes up arguments and calls invoke.  However, we have enough information to make something much much faster when the callee gets Ion-compiled: we can create a per-exit (an exit is a pair (callee, signature)) trampoline directly from asm.js code into Ion code.  Hannes has graciously volunteered to do this.
Comment 1 Hannes Verschore [:h4writer] 2013-05-03 08:31:47 PDT
Created attachment 745174 [details] [diff] [review]
WIP

Sorry about the delay, but I had some fuzz bugs popping in between and also the depending bug took longer, since the code changed dramatically underneath. 

But I'm getting there. This patch enables IM (without argument checks) fastpath for functions that return nothing back. I'm close to finish this. I will immediately enable Baseline too. The codepath is exactly the same. No extra code is needed. I only need to add the checks for return type and make the ARM variant.
Comment 2 Hannes Verschore [:h4writer] 2013-05-07 10:45:47 PDT
Created attachment 746516 [details] [diff] [review]
WIP 2.0

This is a working x64/x86 patch. Very close to landing. Only needs ARM now. Doing that tomorrow. But if you could already give some feedback on how I got this working, that would be appreciated. Thanks!
Comment 3 Hannes Verschore [:h4writer] 2013-05-08 08:25:19 PDT
Created attachment 746970 [details] [diff] [review]
WIP 3.0

ARM is now working too. So the hardest and most important parts are done. Only some minor polishing needed and adding some testcases for edge cases I encountered.

Running a try build to be sure I'm not breaking anything:
https://tbpl.mozilla.org/?tree=Try&rev=a5b4a71f9a37
Comment 4 Hannes Verschore [:h4writer] 2013-05-13 04:40:52 PDT
Created attachment 748754 [details] [diff] [review]
Patch

The TODO's is just adding 2 testcases. I'll do that after the review.
Comment 5 Douglas Crosher [:dougc] 2013-05-15 01:43:33 PDT
(In reply to Hannes Verschore [:h4writer] from comment #4)
> Created attachment 748754 [details] [diff] [review]
> Patch
> 
> The TODO's is just adding 2 testcases. I'll do that after the review.

The ARM support does not compile - issues in GenerateOOLConvert.
I can help fix the issues and test, and need to rebase some other
work anyway, but could you please confirm that this is the most
recent patch?
Comment 6 Hannes Verschore [:h4writer] 2013-05-15 06:38:51 PDT
(In reply to Douglas Crosher [:dougc] from comment #5)
> The ARM support does not compile - issues in GenerateOOLConvert.
> I can help fix the issues and test, and need to rebase some other
> work anyway, but could you please confirm that this is the most
> recent patch?

Oh really. I probably introduced a small issue with the last changes. Since the previous version was tested on on ARM. I'll look at it this evening or tomorrow.
Comment 7 Hannes Verschore [:h4writer] 2013-05-18 10:10:57 PDT
Created attachment 751369 [details] [diff] [review]
Revert bug 871016

Ah I found the issue why this doesn't compile on ARM anymore. Two needed functions are ifdef-out, because they were unused on ARM. See bug 871016. So I need to revert that when pushing.
Comment 8 Luke Wagner [:luke] 2013-05-23 18:32:23 PDT
Comment on attachment 748754 [details] [diff] [review]
Patch

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

This is looking great, I think you found a really nice clean strategy.  I almost got through the patch, but I'm afraid I found a crash running bananabread (http://kripken.github.com/misc-js-benchmarks/banana/benchmark.html).  The crashing pc is:
=> 0x7ffef5ca7c0a:	mov    (%r15,%rax,1),%eax
so, a heap load.  $rax is sane, but $r15 is 0.  So what I assume is happening is that %r15 (the heap base pointer) is not getting restored after a call out to Ion.

Clearly, the asm.js ffi tests are amiss; could you add one that would have caught this (without requiring --ion-eager would be nice).  You might also want to test http://unrealengine.com/html5.

::: js/src/ion/AsmJS.cpp
@@ +5149,5 @@
> +    if (!script->hasIonScript())
> +        return true;
> +
> +    // Currently we can't rectify arguments. Therefore disabling if argc is too low.
> +    // TODO: add testcase for this.

mmm yes, do that :)

@@ +5158,5 @@
> +    AsmJSActivation *activation = cx->runtime->mainThread.asmJSActivationStackFromAnyThread();
> +    if (!activation)
> +        return false;
> +    const AsmJSModule &module = activation->module();
> +    

You should be able to remove this test and write:
const AsmJSModule &module = cx->runtime->mainThread.asmJSActivationFromOwnerThread()->module();

(also delete trailing whitespace)

@@ +5159,5 @@
> +    if (!activation)
> +        return false;
> +    const AsmJSModule &module = activation->module();
> +    
> +#if DEBUG

Usually it's written #ifdef DEBUG

@@ +5236,5 @@
>      return true;
>  }
>  
> +static int32_t
> +ValueToInt32(JSContext *cx, Value *val)

Could these two helpers be moved farther down, right above the function that calls them?

@@ +5550,5 @@
> +    // Arguments are in the following order on the stack:
> +    // descriptor | callee | argc | this | arg1 | arg2 | ...
> +
> +    // Reserve and align space for the arguments
> +    unsigned argBytes = 3 * sizeof(size_t) + (1 + exit.argTypes().length()) * sizeof(Value);

I wish there was some ComputeArgBytes() shared with other parts of Ion so that if the layout changes grepping would point here.

@@ +5553,5 @@
> +    // Reserve and align space for the arguments
> +    unsigned argBytes = 3 * sizeof(size_t) + (1 + exit.argTypes().length()) * sizeof(Value);
> +    unsigned alreadyPushed = AlignmentAtPrologue + masm.framePushed();
> +    unsigned stackDec = AlignBytes(alreadyPushed + argBytes, StackAlignment) - alreadyPushed;
> +    masm.setFramePushed(0);

Could you reuse StackDecrementForCall?  Also, although StackDecrementForCall does observe masm.framePushed(), it should be 0 on entry to Generate*Exit (I just tested).  To make this more clear can you hoist masm.setFramePushed(0) to right after the "MacroAssembler &masm = m.masm()" line above in this and GenerateFFIInterpreterExit?

@@ +5684,5 @@
> +
> +    // Generate the fast path
> +    masm.align(CodeAlignment);
> +    m.setIonExitOffset(exitIndex);
> +    GenerateFFIIonMonkeyExit(m, exit, exitIndex, throwLabel);

Perhaps just "IonExit"... I don't usually see us write out "Monkey".

::: js/src/ion/AsmJS.h
@@ +123,5 @@
>  #endif
>  
> +struct AsmJSModuleExit {
> +    const AsmJSModule *module;
> +    size_t exit;

Could you rename this 'exitIndex'?

::: js/src/ion/AsmJSModule.h
@@ +717,5 @@
>      const PostLinkFailureInfo &postLinkFailureInfo() const {
>          return postLinkFailureInfo_;
>      }
> +
> +    void detachIonCompilation(size_t id) const {

Could you rename 'id' to 'exitIndex'?

::: js/src/ion/Ion.cpp
@@ +909,5 @@
> +    for (size_t i = 0; i < moduleExits.length(); i++) {
> +        AsmJSModuleExit exit = moduleExits[i];
> +        exit.module->detachIonCompilation(exit.exit);
> +    }
> +    moduleExits.clearAndFree();

I think you are doing the "clearAndFree" because ~Vector isn't called?  It's a bit creepy and asking for future trouble to have objects' destructors not be called; it'd be nice (and save 3 words in the common case) to be more clear about this and store a Vector* in the IonScript (lazily malloc'd on first append).
Comment 9 Hannes Verschore [:h4writer] 2013-06-06 05:58:19 PDT
Created attachment 759092 [details] [diff] [review]
patch b (improvements + fix crash)

This should cover the remarks. I also finally found where the crash was and what I forgot. I did the fix quickly (in c code) and this can still get optimized (doing in asm). This bug has been open for way to long, so maybe it would make sense to land this now, and transform to asm in follow-up? As a result we lose some performance ...

Small benchmark:
currently: 147ms
asm.js -> ion: 24ms
ion -> ion: 12ms
Comment 10 Luke Wagner [:luke] 2013-06-06 07:32:24 PDT
(In reply to Hannes Verschore [:h4writer] from comment #9)
Thanks and great work!  I don't understand what you mean by "land this now, and transform to asm in a follow-up"?
Comment 11 Hannes Verschore [:h4writer] 2013-06-06 07:36:12 PDT
(In reply to Luke Wagner [:luke] from comment #10)
> (In reply to Hannes Verschore [:h4writer] from comment #9)
> Thanks and great work!  I don't understand what you mean by "land this now,
> and transform to asm in a follow-up"?

Oh I was talking about the two functions "EnableActivation" and "DisableActivation". Currently I just do an ABICall, but I think it is possible to do this in ASM. I have all code, I only need to script a way to get "cx->fp()" in asm. So I could just do that now or just land this and do this in follow-up bug.
Comment 12 Luke Wagner [:luke] 2013-06-06 07:37:24 PDT
Ohh, I get it.  Sure, it makes sense to do that incrementally.  The improvement is already quite large.
Comment 13 Hannes Verschore [:h4writer] 2013-06-07 17:08:27 PDT
Created attachment 760019 [details] [diff] [review]
Folded patch applies on trunk
Comment 14 Luke Wagner [:luke] 2013-06-07 18:50:26 PDT
Comment on attachment 760019 [details] [diff] [review]
Folded patch applies on trunk

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

Really excited to have this, but strangely, I am still seeing crashes when running unrealengine.com/html5 and http://kripken.github.com/misc-js-benchmarks/banana/benchmark.html.

If I could make a general request, could you add some tests of the various interesting cases with FFI calls (when the callee gets ion compiled) in jit-tests/asm.js/testFFI.js ?

::: js/src/ion/AsmJS.cpp
@@ +5227,5 @@
> +    if (!script->hasIonScript())
> +        return true;
> +
> +    // Currently we can't rectify arguments. Therefore disabling if argc is too low.
> +    // TODO: add testcase for this.

Could you add this testcase?

@@ +5234,5 @@
> +
> +    // Get the corresponding AsmJsModule
> +    const AsmJSModule &module =
> +        cx->runtime->mainThread.asmJSActivationStackFromOwnerThread()->module();
> +    

trailing whitespace

@@ +5414,5 @@
>      i++;
>      JS_ASSERT(i.done());
>  
>      // Make the call, test whether it succeeded, and extract the return value.
> +    //AssertStackAlignment(masm);

Can you comment this back in?

@@ +5599,5 @@
> +      case Use::ToInt32:
> +          masm.call(ImmWord(JS_FUNC_TO_DATA_PTR(void *, &ValueToInt32)));
> +          masm.branchTest32(Assembler::Zero, ReturnReg, ReturnReg, throwLabel);
> +          masm.unboxInt32(Address(StackPointer, offsetToArgv), ReturnReg);
> +          //TODO: add testcase for ToInt32

Yes, please do, because I think you are missing a break here.  (In general, can you make an FFI test for all the variations on weird return types?)

@@ +5617,5 @@
> +    masm.freeStack(stackDec);
> +}
> +
> +void
> +EnableActivation(AsmJSActivation *activation) {

Can you make this and DisableActivation static and put the { on a new line?

@@ +5683,5 @@
> +
> +    // 4. |this| value
> +    masm.storeValue(UndefinedValue(), Address(StackPointer, 3 * sizeof(size_t)));
> +
> +    // 5. Fill the arguments

This whole step exactly duplicates the "Fill the argument array" step in GenerateFFIInterpreterExit.  Could you hoist out a FillArgumentArray (taking exitToArgv as a parameter).

@@ +5715,5 @@
> +
> +    Label ionFailed, done, oolConvert;
> +
> +    // Get the pointer to the ion code
> +    Label *uncompiled = NULL;

Could this be written:

  // Get the pointer to the ion code

  Label done, oolConvert;
  Label *maybeDebugBreakpoint;

(so that the comment looks like it is describing the whole section, not just the label)?

@@ +5718,5 @@
> +    // Get the pointer to the ion code
> +    Label *uncompiled = NULL;
> +
> +#ifdef DEBUG
> +    uncompiled = &ionFailed;

Can you move "Label ionFailed;" inside the #idef DEBUG and wrap the masm.bind(&ionFailed); masm.breakpoint() in #ifdef DEBUG below?

@@ +5733,5 @@
> +    masm.callWithABI(JS_FUNC_TO_DATA_PTR(void *, EnableActivation));
> +    masm.pop(scratch);
> +
> +    // 2. Call
> +    //AssertStackAlignment(masm);

Can you comment this back in (unless Ion expects different alignment, and then could you assert that?)

@@ +5759,5 @@
> +      case Use::AddOrSub:
> +        JS_NOT_REACHED("Should have been a type error");
> +    }
> +
> +    //masm.breakpoint();

rm

@@ +5783,5 @@
> +    MacroAssembler &masm = m.masm();
> +
> +    // Generate the slow path through the interpreter
> +    masm.align(CodeAlignment);
> +    m.setInterpExitOffset(exitIndex);

Can you move these two lines into GenerateFFIInterpreterExit (and similarly for GenerateFFIIonExit) for symmetry with the other Generate* functions?

::: js/src/ion/AsmJS.h
@@ +122,5 @@
>      void setCurrentThread();
>  };
>  #endif
>  
> +struct AsmJSModuleExit {

{ on new line

::: js/src/ion/AsmJSModule.h
@@ +536,5 @@
>      // section.
>      struct ExitDatum
>      {
>          uint8_t *exit;
> +        unsigned id;

I think we can remove this field.  Instead, you can add a method to AsmJSModule:
  exitDatumToExitIndex(ExitDatum *exit) {
     ExitDatum *first = &exitIndexToGlobalDatum(0);
     JS_ASSERT(exit >= first && exit < first + numExits());
     return exit - first;
  }
and use this where you are using 'id' now.

::: js/src/ion/Ion.cpp
@@ +914,5 @@
> +bool
> +IonScript::addAsmJSModule(JSContext *cx, AsmJSModuleExit exit)
> +{
> +    if (!moduleExits)
> +        moduleExits = cx->new_<Vector<AsmJSModuleExit> >(cx);

Need "if (!moduleExits) return false"

::: js/src/ion/IonCode.h
@@ +247,5 @@
>      // Number of times we tried to enter this script via OSR but failed due to
>      // a LOOPENTRY pc other than osrPc_.
>      uint32_t osrPcMismatchCounter_;
>  
> +    Vector<AsmJSModuleExit> *moduleExits;

Could you rename this field "dependentAsmJSModules" and provide a comment "If non-null, the list of AsmJSModules that contain an optimized call directly into this IonScript."  Similarly, could you s/AsmJSModuleExit/DependentAsmJSModule/?

@@ +289,5 @@
>      JSScript **callTargetList() {
>          return (JSScript **) &bottomBuffer()[callTargetList_];
>      }
> +    bool addAsmJSModule(JSContext *cx, AsmJSModuleExit exit);
> +    void removeAsmJSModule(AsmJSModuleExit exit) {

Along with the above renaming: addDependentAsmJSModule/removeDependentAsmJSModule.
Comment 15 Hannes Verschore [:h4writer] 2013-06-09 16:01:14 PDT
(In reply to Luke Wagner [:luke] from comment #14)
> Really excited to have this, but strangely, I am still seeing crashes when
> running unrealengine.com/html5 and
> http://kripken.github.com/misc-js-benchmarks/banana/benchmark.html.

Thanks for the review. Made a small fault (clobbering register before saving state). Will post an updated patch Monday with comments addressed and not failing the tests.
Comment 16 Hannes Verschore [:h4writer] 2013-06-10 16:29:17 PDT
Update: that wasn't the only fault left. So still bug hunting, before I can request new review
Comment 17 Hannes Verschore [:h4writer] 2013-06-11 06:29:36 PDT
Created attachment 760873 [details] [diff] [review]
Fixes for browser/arm + comments addressed

Finally! After debugging and building way too long.
- Added the testcases
- Passes jittests on ARM (emulator + chromebook)
- Passes jittests on x86
- Runs bananabread in browser
- Fixed the raised comments

Just ping if you want a combined patch.
Comment 18 Hannes Verschore [:h4writer] 2013-06-11 08:56:34 PDT
Created attachment 760958 [details] [diff] [review]
Combined patch

Actually I can already combine them. This also applies on trunk. There were two small issues when updating. I also did a try run. Just to be sure (esp. for ARM).

https://tbpl.mozilla.org/?tree=Try&rev=b78567bcfe00
Comment 19 Luke Wagner [:luke] 2013-06-11 09:26:33 PDT
Comment on attachment 760958 [details] [diff] [review]
Combined patch

Great job!  Thanks for sticking with it :)
Comment 20 Hannes Verschore [:h4writer] 2013-06-12 07:26:55 PDT
Created attachment 761432 [details] [diff] [review]
Fix for the stack changes overnight

Overnight Jan introduced some stack changes, resulting in ffi not applying clean anymore. Jan suggested to solve it this way.
Comment 21 Hannes Verschore [:h4writer] 2013-06-12 07:37:08 PDT
Comment on attachment 761432 [details] [diff] [review]
Fix for the stack changes overnight

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

::: js/src/ion/AsmJS.cpp
@@ +5623,5 @@
>  static void
>  EnableActivation(AsmJSActivation *activation)
>  {
>      JSContext *cx = activation->cx();
> +    cx->fp()->setRunningInJit();

Jan: this should be removed

@@ +5631,5 @@
>  static void
>  DisableActivation(AsmJSActivation *activation)
>  {
>      JSContext *cx = activation->cx();
> +    cx->fp()->clearRunningInJit();

Jan: this should be removed
Comment 22 Hannes Verschore [:h4writer] 2013-06-12 13:01:22 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/2810e80e1393
Comment 23 Ed Morley [:emorley] 2013-06-13 02:35:28 PDT
https://hg.mozilla.org/mozilla-central/rev/2810e80e1393
Comment 24 Douglas Crosher [:dougc] 2013-06-17 02:46:52 PDT
This appears to be causing bugs on the ARM.  The Bananabench test has corrupted display output on nightly (Android).  A build just after the above patch landed crashes a debug build, and a build just prior runs fine.  Shall take a look unless someone beats me to it.  Might it be possible to disable some of the new paths for the ARM for now?
Comment 25 Hannes Verschore [:h4writer] 2013-06-17 02:48:52 PDT
Could you try with the patch in bug 883490 applied? That is still a fall-out from landing. Should get pushed tonight.
Comment 26 Douglas Crosher [:dougc] 2013-06-17 05:37:43 PDT
The patch in bug 883490 changes the output and prevents the crash, but the output is still corrupt and now the script seems to stop (without freezing the whole browser).
Comment 27 Douglas Crosher [:dougc] 2013-06-17 22:04:42 PDT
Noted an assertion failure in TryEnablingIon. The call to TryEnablingIon is from InvokeFromAsmJS_Ignore.

JS_ASSERT(types::TypeScript::ThisTypes(script)->hasType(types::Type::UndefinedType()));

Should this not have happened, or should it just be giving up on enabling the fast exit to Ion code in this case?
Comment 28 Hannes Verschore [:h4writer] 2013-06-18 01:38:16 PDT
(In reply to Douglas Crosher [:dougc] from comment #27)
> Noted an assertion failure in TryEnablingIon. The call to TryEnablingIon is
> from InvokeFromAsmJS_Ignore.
> 
> JS_ASSERT(types::TypeScript::ThisTypes(script)->hasType(types::Type::
> UndefinedType()));
> 
> Should this not have happened, or should it just be giving up on enabling
> the fast exit to Ion code in this case?

Asm.js ffi calls have always "undefined" as |this| value. So this must always be true. I added this check actually only to convince myself there aren't weird interactions. So if you have a testcase that would awesome!
Comment 29 Douglas Crosher [:dougc] 2013-06-18 06:58:07 PDT
(In reply to Hannes Verschore [:h4writer] from comment #28)
> (In reply to Douglas Crosher [:dougc] from comment #27)
> > Noted an assertion failure in TryEnablingIon. The call to TryEnablingIon is
> > from InvokeFromAsmJS_Ignore.
> > 
> > JS_ASSERT(types::TypeScript::ThisTypes(script)->hasType(types::Type::
> > UndefinedType()));
> > 
> > Should this not have happened, or should it just be giving up on enabling
> > the fast exit to Ion code in this case?
> 
> Asm.js ffi calls have always "undefined" as |this| value. So this must
> always be true. I added this check actually only to convince myself there
> aren't weird interactions. So if you have a testcase that would awesome!

Hit this assertion running the Citadel demo on a x64 build.  The function it fails on is _alBufferData from http://www.unrealengine.com/html5/UDKGame-Browser-Shipping.js lineno 5149.  Even if the 'this' type assertion is ignored, it fails on the following argument type checks.

Is this type information persistent?  Might it have been cleared by a GC?

Btw: the ARM still has issues even if the new fast path to Ion is disabled by always giving up in TryEnablingIon.  The BananaBench output looks a lot better, but there is still some corruption. Have not confirmed that this corruption is caused by the above patch, but if so then checking the slow path might be a good place to start.
Comment 30 Hannes Verschore [:h4writer] 2013-06-18 16:23:47 PDT
(In reply to Douglas Crosher [:dougc] from comment #29)
> (In reply to Hannes Verschore [:h4writer] from comment #28)
> > (In reply to Douglas Crosher [:dougc] from comment #27)
> > > Noted an assertion failure in TryEnablingIon. The call to TryEnablingIon is
> > > from InvokeFromAsmJS_Ignore.
> > > 
> > > JS_ASSERT(types::TypeScript::ThisTypes(script)->hasType(types::Type::
> > > UndefinedType()));
> > > 
> > > Should this not have happened, or should it just be giving up on enabling
> > > the fast exit to Ion code in this case?
> > 
> > Asm.js ffi calls have always "undefined" as |this| value. So this must
> > always be true. I added this check actually only to convince myself there
> > aren't weird interactions. So if you have a testcase that would awesome!
> 
> Hit this assertion running the Citadel demo on a x64 build.  The function it
> fails on is _alBufferData from
> http://www.unrealengine.com/html5/UDKGame-Browser-Shipping.js lineno 5149. 
> Even if the 'this' type assertion is ignored, it fails on the following
> argument type checks.
> Is this type information persistent?  Might it have been cleared by a GC?
I find it very interesting, since I added those assertation for my own, but was almost sure they cannot hit. Can you create a bug for it?

> Btw: the ARM still has issues even if the new fast path to Ion is disabled
> by always giving up in TryEnablingIon. 
That could possible be bug 883626. Since disabling TryEnablingIon also kept that bug around.
Comment 31 Douglas Crosher [:dougc] 2013-06-19 00:00:31 PDT
(In reply to Hannes Verschore [:h4writer] from comment #30)
> (In reply to Douglas Crosher [:dougc] from comment #29)
...
> > Btw: the ARM still has issues even if the new fast path to Ion is disabled
> > by always giving up in TryEnablingIon. 
> That could possible be bug 883626. Since disabling TryEnablingIon also kept
> that bug around.

It looks like the corruption seen when running BananaBench on the ARM build, with TryEnablingIon disabled, is a pre-existing issue, and not something new introduced here.  ARM builds many weeks before this bug's patch landed show the same corruption - at least the output is different to the other ports and looks like logical errors that could be attributed to the compiled JS rather than just rendering pipeline issues.  Shall explore this as a separate issue.

A recent ARM build, after 883626 landed, and with TryEnablingIon disabled, is unchanged - it does not address the ARM issue, but does not seem to make things worse.  Thus, it might help if the fast path could be disabled for the ARM, with a simple patch to TryEnablingIon, so that the ARM specific issues with the fast path can be explored with less pressure?

Note You need to log in before you can comment on or make changes to this bug.