915495 - lack of textures when zooming out on New Google Maps

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Alice0775 White]

This is spun off from Bug 915301 Comment 10

Steps To Reproduce:
1. Open URL
2. Enable New Maps
3. Zoom-out

Actual Results:
No textures rendered

Regression window(m-i)
Good:
http://hg.mozilla.org/integration/mozilla-inbound/rev/0452b5b504d0
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:26.0) Gecko/20100101 Firefox/26.0 ID:20130910032034
Bad:
http://hg.mozilla.org/integration/mozilla-inbound/rev/19918a47a06f
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:26.0) Gecko/20100101 Firefox/26.0 ID:20130910044250
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=0452b5b504d0&tochange=19918a47a06f

Suspected:
19918a47a06f	Nicolas Silva — Bug 913821 - Fix the TextureHost linked list. r=sotaro

Comment 1

[Alice0775 White]

Attachment: screenshot

Comment 2

[Alice0775 White]

(In reply to Alice0775 White from comment #0)
> This is spun off from Bug 915301 Comment 10
> 
> 
> Suspected:
> 19918a47a06f	Nicolas Silva — Bug 913821 - Fix the TextureHost linked list.
> r=sotaro

The suspected bug is wrong.

I tried in local build to find regression.
Last Good : 4ab57d0318ff
First Bad : 255093e2f430

Regressed by:
255093e2f430	Shu-yu Guo — Bug 914478 - Fix checking for error in setElemTryCache. (r=jandem)

And I also confirmed that setting javascript.options.ion.content = false fixed the problem

Comment 3

[Jan de Mooij [:jandem]]

This could be a pre-existing issue that was exposed by that patch, but it could also be a problem with the new typed array stubs :)

Comment 4

[Shu-yu Guo [:shu]]

Ouch! Looking at this ASAP...

Comment 5

[Benjamin Bouvier [:bbouvier] (inactive)]

I will try to investigate quickly too, it might be a fallout of Float32 landing.

Comment 6

[Shu-yu Guo [:shu]]

Attachment: v0

I didn't know about bbouvier's float32 patch when I was writing the typed array IC patch, so everything that goes through the IC gets conversions wrong.

This refactors the masm methods to address this. Also 2 drive-by fixes
 - One place where I forgot to clamp an int32
 - An existing bug in masm.clampDoubleToUint8 where we never undo the in-place +0.5 to the input register, causing chained assignments like (a[i] = a[j] = source[i]) to mysteriously assign different values to a[i] and a[j].

Sorry this patch is so big; the bug seemed urgent and so was in a hurry to get it done, didn't have to split it up.

Benjamin, could you see if you can write some tests for these paths?

Comment 7

[Shu-yu Guo [:shu]]

didn't have time to split it up*

Comment 8

[Jan de Mooij [:jandem]]

Comment on attachment 804287 [details] [diff] [review]
v0

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

This looks good at first glance, but I think Benjamin is a better reviewer for the float32 code :)

Comment 10

[Shu-yu Guo [:shu]]

Attachment: v0 delta

v0 was outdated; this delta reverts the changes I made to storeToTypedFloatArray to use different named paths for floats and doubles.

Comment 11

[Shu-yu Guo [:shu]]

Attachment: v0 delta

Uploaded wrong one.

Comment 12

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 804287 [details] [diff] [review]
v0

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

r=me with the fixed clobbering / comment issue.

Very nice patch, thanks! I like the refactoring you made.

I thought I could avoid adding boxing and unboxing specializations for Float32, as they are (or more precisely, were) not supposed to flow into boxing operations. I didn't think of this usage though and it is legit to introduce it.

There might be some clobbering issues, unless the BoxFloatingPoint operation clobbers its input, in which case we'd need a comment.

::: js/src/jit/CodeGenerator.cpp
@@ +5748,5 @@
>  
>      return true;
>  }
>  
>  // An out-of-line path to convert a boxed int32 to a double.

nit: comment: "double" -> "float32 or double", or "floating point number", or anything that would make sense.

::: js/src/jit/IonMacroAssembler.cpp
@@ +514,5 @@
>          ma_bic(Imm32(1), output, NoSetCond, Zero);
> +        // We added 0.5 in place, but the input register might be used again,
> +        // so undo it.
> +        ma_vimm(0.5, ScratchFloatReg);
> +        ma_vsub(input, ScratchFloatReg, input);

Nice catch! Did it appear in the extent of this test case? It might be useful to bake a specific test for this situation.

Also, as a follow-up, it looks like this code is ARM specific and should be in the ARM macro assembler.

@@ +1760,5 @@
>        case MIRType_Boolean:
>        case MIRType_Int32:
>          if (src.typedReg().gpr() != output)
>              move32(src.typedReg().gpr(), output);
> +        if (src.type() == MIRType_Int32 && behavior == IntConversion_ClampToUint8)

I am not really familiar with the callers. Can there be other behaviors to take into account here?

::: js/src/jit/IonMacroAssembler.h
@@ +1004,5 @@
>      void tracelogStop();
>      void tracelogLog(TraceLogging::Type type);
>  #endif
>  
> +#define FLOATING_POINT_OP_2(method, type, arg1d, arg1f, arg2)   \

Meta programming FTW! However, I think the name of the macro is confusing, especially the OP_2 part (while there are indeed only two involved arguments, three names start with the |arg| prefix). Could you rename it please?

::: js/src/jit/arm/CodeGenerator-arm.cpp
@@ +1272,5 @@
>      const LAllocation *in = box->getOperand(0);
>  
> +    FloatRegister reg = ToFloatRegister(in);
> +    if (box->type() == MIRType_Float32)
> +        masm.convertFloatToDouble(reg, reg);

You might clobber the input register in this case, which can be used by other instructions that would assume that the register contains a Float32.

::: js/src/jit/x64/CodeGenerator-x64.cpp
@@ +85,5 @@
>      const LDefinition *result = box->getDef(0);
>  
> +    if (IsFloatingPointType(box->type())) {
> +        if (box->type() == MIRType_Float32)
> +            masm.convertFloatToDouble(ToFloatRegister(in), ToFloatRegister(in));

Same remark regarding clobbering. Using the ScratchFloatReg as a temporary can help here.

::: js/src/jit/x86/CodeGenerator-x86.cpp
@@ +124,5 @@
>      const ValueOperand out = ToOutValue(box);
>  
> +    FloatRegister reg = ToFloatRegister(in);
> +    if (box->type() == MIRType_Float32)
> +        masm.convertFloatToDouble(reg, reg);

Ditto.

Comment 13

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 804693 [details] [diff] [review]
v0 delta

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

Cool!

Comment 14

[Shu-yu Guo [:shu]]

Waiting on https://tbpl.mozilla.org/?tree=Try&rev=34c4b682b578

Comment 15

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Landed in https://hg.mozilla.org/integration/mozilla-inbound/rev/422937706171

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/e66d28e87ce1 for breaking at least Android builds.

In file included from ../../../js/src/jit/IonMacroAssembler.h:19:0,
                 from ../../../js/src/jit/BaselineJIT.h:20,
                 from ../../../js/src/jsscriptinlines.h:13,
                 from ../../../js/src/vm/ArgumentsObject-inl.h:14,
                 from ../../../js/src/vm/ArgumentsObject.cpp:7:
../../../js/src/jit/arm/MacroAssembler-arm.h: In member function 'void js::jit::MacroAssemblerARMCompat::moveFloat(js::jit::FloatRegister, js::jit::FloatRegister)':
../../../js/src/jit/arm/MacroAssembler-arm.h:1461:84: error: no matching function for call to 'js::jit::MacroAssemblerARMCompat::ma_vmov(js::jit::VFPRegister, js::jit::VFPRegister)'
../../../js/src/jit/arm/MacroAssembler-arm.h:1461:84: note: candidate is:
In file included from ../../../js/src/jit/IonMacroAssembler.h:19:0,
                 from ../../../js/src/jit/BaselineJIT.h:20,
                 from ../../../js/src/jsscriptinlines.h:13,
                 from ../../../js/src/vm/ArgumentsObject-inl.h:14,
                 from ../../../js/src/vm/ArgumentsObject.cpp:7:
../../../js/src/jit/arm/MacroAssembler-arm.h:324:10: note: void js::jit::MacroAssemblerARM::ma_vmov(js::jit::FloatRegister, js::jit::FloatRegister, js::jit::Assembler::Condition)
../../../js/src/jit/arm/MacroAssembler-arm.h:324:10: note:   no known conversion for argument 1 from 'js::jit::VFPRegister' to 'js::jit::FloatRegister'
In file included from ../../../js/src/jit/BaselineJIT.h:20:0,
                 from ../../../js/src/jsscriptinlines.h:13,
                 from ../../../js/src/vm/ArgumentsObject-inl.h:14,
                 from ../../../js/src/vm/ArgumentsObject.cpp:7:
../../../js/src/jit/IonMacroAssembler.h: In member function 'void js::jit::MacroAssembler::loadConstantFloatingPoint(double, float, js::jit::FloatRegister, js::jit::MIRType)':
../../../js/src/jit/IonMacroAssembler.h:1019:119: error: 'loadConstantFloat32' was not declared in this scope

https://tbpl.mozilla.org/php/getParsedLog.php?id=27869154&tree=Mozilla-Inbound#error0

Comment 16

[Douglas Crosher [:dougc]]

Attachment: Fix moveFloat. Combined patch.

The patches would not compile without support from bug 900756,
but with both applied and some minor rebasing and fixes, the ARM
backend passed the standard jit-tests.  The map does not
appear corrupted with these changes when tests on Android ARM.

Comment 17

[Shu-yu Guo [:shu]]

Doug, I pushed a fixed patch last night.

https://hg.mozilla.org/integration/mozilla-inbound/rev/a43be719866e

Comment 18

[Douglas Crosher [:dougc]]

The ARM backend is crashing (probably assertion failures in the debug build),
perhaps due to these changes, or perhaps just the general float32 changes.

It is possible to crash the x86 build by changing allowFloat32Optimizations
to return false, which might test some of the failing ARM paths, and perhaps
this would help debugging.

For example the jit-tests asm.js/testZOOB.js fails with:
Assertion failure: ins->value()->type() == MIRType_Float32, at js/src/jit/Lowering.cpp:2442

Might the code base have past the point of no return on the float32
support, and might the ARM backend be in better shape by landing the
ARM float32 support in bug 900756?  Note, even with both patch sets
applied I am still seeing new ARM crashes, but the jit-tests pass.

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/a43be719866e

Comment 20

[Douglas Crosher [:dougc]]

With allowFloat32Optimizations modified to return false on the x86
the following jit-tests crash with the assertion failure:
Assertion failure: ins->value()->type() == MIRType_Float32, at /home/src/b2g/src/js/src/jit/Lowering.cpp:2442

    --baseline-eager tests/asm.js/testZOOB.js
    --ion-eager --ion-check-range-analysistests/asm.js/testZOOB.js
    --ion-eager tests/asm.js/testZOOB.js
    tests/asm.js/testZOOB.js

    --ion-eager tests/basic/bug652054.js
    --ion-eager --ion-check-range-analysis tests/basic/bug652054.js

    --ion-eager tests/basic/test586387.js
    --ion-eager --ion-check-range-analysis tests/basic/test586387.js

    --ion-eager --ion-check-range-analysis tests/basic/testTypedArrayArith.js
    --ion-eager tests/basic/testTypedArrayArith.js

    --ion-eager --ion-check-range-analysis tests/basic/testTypedArrays.js
    --ion-eager tests/basic/testTypedArrays.js

    --ion-eager --ion-check-range-analysis tests/ion/bug750588.js
    --ion-eager tests/ion/bug750588.js

    --ion-eager tests/ion/bug851792.js
    --ion-eager --ion-check-range-analysis tests/ion/bug851792.js

    --ion-eager --ion-check-range-analysis tests/ion/bug915301.js
    --baseline-eager tests/ion/bug915301.js
    tests/ion/bug915301.js
    --ion-eager tests/ion/bug915301.js

    --baseline-eager ttests/ion/testFloat32.js
    --ion-eager tests/ion/testFloat32.js
    --ion-eager --ion-check-range-analysis tests/ion/testFloat32.js
    tests/ion/testFloat32.js

    --ion-eager tests/ion/typed-arrays-1.js
    --ion-eager --ion-check-range-analysis tests/ion/typed-arrays-1.js

    --ion-eager --ion-check-range-analysis tests/jaeger/testSetTypedFloatArray.js
    --ion-eager tests/jaeger/testSetTypedFloatArray.js


Further, the following jit-tests fail with what appears to be numerical errors:
    tests/ion/setelem-float32-typedarray-ic.js
    --baseline-eager tests/ion/setelem-float32-typedarray-ic.js

Comment 21

[Shu-yu Guo [:shu]]

(In reply to Douglas Crosher [:dougc] from comment #20)
> With allowFloat32Optimizations modified to return false on the x86

I'm not understanding -- will this ever be the case?

Comment 22

[Shu-yu Guo [:shu]]

In any case, perhaps bbouvier could look at the new crashes.

Comment 23

[Douglas Crosher [:dougc]]

(In reply to Shu-yu Guo [:shu] from comment #21)
> (In reply to Douglas Crosher [:dougc] from comment #20)
> > With allowFloat32Optimizations modified to return false on the x86
> 
> I'm not understanding -- will this ever be the case?

Perhaps not on the x86 but this is currently the case for the ARM
and there are a lot of jit-test failures.  I just thought it would
be easier to debug some of these on the x86.

There are other classes of ARM specific jit-test failures that I am
still exploring: an unused label assertion failure, and branches
that are out of the instruction encoding range.

Comment 24

[Shu-yu Guo [:shu]]

(In reply to Douglas Crosher [:dougc] from comment #23)
> Perhaps not on the x86 but this is currently the case for the ARM
> and there are a lot of jit-test failures.  I just thought it would
> be easier to debug some of these on the x86.
> 
> There are other classes of ARM specific jit-test failures that I am
> still exploring: an unused label assertion failure, and branches
> that are out of the instruction encoding range.

Yes, I see. Could you or bbouvier take those failures? I probably won't be able to get to them until mid next week.