Closed Bug 915495 Opened 6 years ago Closed 6 years ago

lack of textures when zooming out on New Google Maps

Categories

(Core :: JavaScript Engine, defect)

26 Branch
x86_64
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla26
Tracking Status
firefox25 --- unaffected
firefox26 + fixed

People

(Reporter: alice0775, Assigned: shu)

References

()

Details

(Keywords: regression)

Attachments

(4 files, 1 obsolete file)

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
Summary: lack of textures when zooming out → lack of textures when zooming out on New Google Maps
Attached image screenshot
(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
Assignee: nobody → general
Blocks: 914478
No longer blocks: 913821
Component: Graphics: Layers → JavaScript Engine
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 :)
Flags: needinfo?(shu)
Ouch! Looking at this ASAP...
Flags: needinfo?(shu)
I will try to investigate quickly too, it might be a fallout of Float32 landing.
Attached patch v0Splinter Review
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?
Assignee: general → shu
Attachment #804287 - Flags: review?(jdemooij)
Attachment #804287 - Flags: feedback?(bbouvier)
didn't have time to split it up*
Flags: in-testsuite?
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 :)
Attachment #804287 - Flags: review?(jdemooij)
Attachment #804287 - Flags: review?(bbouvier)
Attachment #804287 - Flags: feedback?(bbouvier)
Attachment #804287 - Flags: feedback+
Duplicate of this bug: 915979
Attached patch v0 delta (obsolete) — Splinter Review
v0 was outdated; this delta reverts the changes I made to storeToTypedFloatArray to use different named paths for floats and doubles.
Attachment #804691 - Flags: review?(bbouvier)
Attached patch v0 deltaSplinter Review
Uploaded wrong one.
Attachment #804691 - Attachment is obsolete: true
Attachment #804691 - Flags: review?(bbouvier)
Attachment #804693 - Flags: review?(bbouvier)
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.
Attachment #804287 - Flags: review?(bbouvier) → review+
Comment on attachment 804693 [details] [diff] [review]
v0 delta

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

Cool!
Attachment #804693 - Flags: review?(bbouvier) → review+
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
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.
Doug, I pushed a fixed patch last night.

https://hg.mozilla.org/integration/mozilla-inbound/rev/a43be719866e
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.
https://hg.mozilla.org/mozilla-central/rev/a43be719866e
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
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
(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?
In any case, perhaps bbouvier could look at the new crashes.
Flags: needinfo?(bbouvier)
(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.
(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.
Flags: needinfo?(bbouvier)
Flags: in-testsuite?
Flags: in-testsuite+
You need to log in before you can comment on or make changes to this bug.