Closed
Bug 915301
Opened 11 years ago
Closed 11 years ago
Google Maps smeared street graphics in current Nightly.
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
mozilla26
People
(Reporter: lh.bennett, Assigned: bbouvier)
References
()
Details
(Keywords: regression)
Attachments
(3 files, 1 obsolete file)
(deleted),
image/jpeg
|
Details | |
(deleted),
patch
|
sstangl
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
terrence
:
review+
|
Details | Diff | Splinter Review |
User Agent: Mozilla/5.0 (Windows NT 6.2; WOW64; rv:26.0) Gecko/20100101 Firefox/26.0 (Beta/Release)
Build ID: 20130911030258
Steps to reproduce:
Google Maps in today's build streaks street graphics all over the map.
Screenshot attached.
Comment 1•11 years ago
|
||
Confirm for: Mozilla/5.0 (X11; Linux x86_64; rv:26.0) Gecko/20100101 Firefox/26.0 ID:20130911030258 CSet: 9e9f74116749
about:buildconfig
Build Machine
bld-centos6-hp-015
Source
Built from http://hg.mozilla.org/mozilla-central/rev/9e9f74116749
Build platform
target
x86_64-unknown-linux-gnu
Build tools
Compiler Version Compiler flags
/usr/bin/ccache /tools/gcc-4.7.3-0moz1/bin/gcc gcc version 4.7.3 (GCC) -Wall -Wpointer-arith -Wdeclaration-after-statement -Werror=return-type -Wtype-limits -Wempty-body -Wsign-compare -Wno-unused -Wcast-align -Wno-error=uninitialized -Wno-error=maybe-uninitialized -Wno-error=deprecated-declarations -std=gnu99 -fgnu89-inline -fno-strict-aliasing -ffunction-sections -fdata-sections -fno-math-errno -pthread -pipe -DNDEBUG -DTRIMMED -g -fprofile-use -fprofile-correction -Wcoverage-mismatch -O3 -fno-omit-frame-pointer
/usr/bin/ccache /tools/gcc-4.7.3-0moz1/bin/g++ gcc version 4.7.3 (GCC) -Wall -Wpointer-arith -Woverloaded-virtual -Werror=return-type -Wtype-limits -Wempty-body -Wsign-compare -Wno-invalid-offsetof -Wcast-align -Wno-error=uninitialized -Wno-error=maybe-uninitialized -Wno-error=deprecated-declarations -fno-exceptions -fno-strict-aliasing -fno-rtti -ffunction-sections -fdata-sections -fno-exceptions -fno-math-errno -std=gnu++0x -pthread -pipe -DNDEBUG -DTRIMMED -g -fprofile-use -fprofile-correction -Wcoverage-mismatch -O3 -fno-omit-frame-pointer
Configure arguments
--enable-update-channel=nightly --enable-update-packaging --with-google-api-keyfile=/builds/gapi.data --enable-crashreporter --enable-release --enable-elf-hack --enable-stdcxx-compat --enable-warnings-as-errors --enable-signmar --enable-profiling --disable-elf-hack --enable-js-diagnostics --with-ccache=/usr/bin/ccache
Status: UNCONFIRMED → NEW
Ever confirmed: true
OS: Windows 8 → All
Hardware: x86_64 → All
Comment 2•11 years ago
|
||
Hardware acceleration is enable.
Comment 3•11 years ago
|
||
Regression widnwow(m-c)
Good:
http://hg.mozilla.org/mozilla-central/rev/be1053dc223b
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:26.0) Gecko/20100101 Firefox/26.0 ID:20130910000748
Bad:
http://hg.mozilla.org/mozilla-central/rev/25bfaa953892
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:26.0) Gecko/20100101 Firefox/26.0 ID:20130910011434
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=be1053dc223b&tochange=25bfaa953892
Regression widnwow(m-i)
Good:
http://hg.mozilla.org/integration/mozilla-inbound/rev/4a6ac4c0ab58
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:26.0) Gecko/20100101 Firefox/26.0 ID:20130909155246
Bad:
http://hg.mozilla.org/integration/mozilla-inbound/rev/a43cf13bd6a6
Mozilla/5.0 (Windows NT 6.1; WOW64; rv:26.0) Gecko/20100101 Firefox/26.0 ID:20130909155549
Pushlog:
http://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=4a6ac4c0ab58&tochange=a43cf13bd6a6
Regressed by:
a43cf13bd6a6 Benjamin Bouvier — Bug 888109: Float32 general optimizations for IonMonkey: framework and arithmetic operations; r=sstangl,nbp
Assignee: nobody → general
Blocks: 888109
tracking-firefox26:
--- → ?
Component: Untriaged → JavaScript Engine
Keywords: regression
Product: Firefox → Core
Version: Trunk → 26 Branch
Comment 4•11 years ago
|
||
FYI,
Setting javascript.options.ion.content;false fixes the problem.
Assignee | ||
Comment 5•11 years ago
|
||
The good parts: Maps use Float32 so it *could* get faster.
The bad parts: there seem to be a conversion failure somewhere.
Interesting, I will investigate this from now. I can reproduce on linux x64, so it's not OS specific. I will try to find whether it's platform specific and reduce the test case.
Assignee: general → bbouvier
Status: NEW → ASSIGNED
Assignee | ||
Comment 6•11 years ago
|
||
Thanks to Sean for the idea: this adds a pass in debug mode that checks that all instructions specialized as Float32 are actually flowing into instructions that can handle them (consumers, instructions specialized as Float32 or ToDouble).
This patch also contains a reduced test case showing the error:
- A phi is created with type Double during ion building.
- This phi is passed as the argument of a MPassArg, later during ion building. This MPassArg is thus specialized as Double.
- During specializePhis, the Phi is respecialized as Float32, but the MPassArg is not updated.
As a matter of fact, float32 arguments flow into a function that expects doubles.
Not flagging for review yet as it could blow up some mochi tests. I am going to try first.
Assignee | ||
Comment 7•11 years ago
|
||
This is ugly and gross, but this is also temporary. This regression is pretty bad and it deserves a smarter fix, that I will do in bug 915479. In the mean while, there might be correctness errors on all websites that use Float32Arrays.
In this patch, we adjust real argument types by hand, as a temporary workaround.
Attachment #803437 -
Flags: review?(terrence)
Comment 8•11 years ago
|
||
Comment on attachment 803437 [details] [diff] [review]
Temporary fix
Review of attachment 803437 [details] [diff] [review]:
-----------------------------------------------------------------
r=me
Attachment #803437 -
Flags: review?(terrence) → review+
Assignee | ||
Comment 9•11 years ago
|
||
This fix makes MPassArg a NoFloatPolicy, preventing it from receiving Float32 as an argument. In this case, Float32 arguments will be converted to a Double. It's cleaner than the previous patch and it even allows some code removal.
Attachment #803437 -
Attachment is obsolete: true
Attachment #803460 -
Flags: review?(terrence)
Assignee | ||
Comment 10•11 years ago
|
||
Alice, do you know if the lack of textures when zooming out is also due to the same cset?
Flags: needinfo?(alice0775)
Comment 11•11 years ago
|
||
Comment on attachment 803460 [details] [diff] [review]
Cleaner fix
Review of attachment 803460 [details] [diff] [review]:
-----------------------------------------------------------------
Much nicer. r=me
Attachment #803460 -
Flags: review?(terrence) → review+
Assignee | ||
Comment 12•11 years ago
|
||
Assignee | ||
Updated•11 years ago
|
Whiteboard: [leave open]
Comment 13•11 years ago
|
||
(In reply to Benjamin Bouvier [:bbouvier] from comment #10)
> Alice, do you know if the lack of textures when zooming out is also due to
> the same cset?
The lack of textures is caused by different cset.
I've filed Bug 915495
Flags: needinfo?(alice0775)
Comment 14•11 years ago
|
||
Assignee | ||
Comment 15•11 years ago
|
||
Comment on attachment 803432 [details] [diff] [review]
Coherency assertions in debug mode, for that not to happen again
Try returned green for the check patch:
https://tbpl.mozilla.org/?tree=Try&rev=cdde9daaba62
Attachment #803432 -
Flags: review?(sstangl)
Comment 16•11 years ago
|
||
Comment on attachment 803432 [details] [diff] [review]
Coherency assertions in debug mode, for that not to happen again
Review of attachment 803432 [details] [diff] [review]:
-----------------------------------------------------------------
::: js/src/jit/IonAnalysis.cpp
@@ +911,5 @@
> + if (mir->shouldCancel("Check Float32 coherency"))
> + return false;
> +
> + for (MDefinitionIterator def(*block); def; def++) {
> + if (def->type() == MIRType_Float32) {
prefer reducing a level of indentation by changing this to a guard:
if (def->type() != MIRType_Float32)
continue;
@@ +915,5 @@
> + if (def->type() == MIRType_Float32) {
> + if (def->isPassArg()) // no check for PassArg as it is broken, see bug 915479
> + continue;
> +
> + for(MUseDefIterator use(*def); use; use++) {
nit: whitespace after "for"
Attachment #803432 -
Flags: review?(sstangl) → review+
Assignee | ||
Comment 17•11 years ago
|
||
Also added a comment for explaining why the check makes sense.
https://hg.mozilla.org/integration/mozilla-inbound/rev/13568a3576cd
Whiteboard: [leave open]
Comment 18•11 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
Reporter | ||
Comment 19•11 years ago
|
||
Should this be considered fixed with Bug 916816 describing the exact same scenario?
Comment 20•11 years ago
|
||
(In reply to Leman Bennett (Omega X) from comment #19)
> Should this be considered fixed with Bug 916816 describing the exact same
> scenario?
Yes, that bug will follow up and is tracked so this regression is correctly fixed in time for release.
tracking-firefox26:
? → ---
You need to log in
before you can comment on or make changes to this bug.
Description
•