Closed Bug 915301 Opened 11 years ago Closed 11 years ago

Google Maps smeared street graphics in current Nightly.

Categories

(Core :: JavaScript Engine, defect)

26 Branch
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla26

People

(Reporter: lh.bennett, Assigned: bbouvier)

References

()

Details

(Keywords: regression)

Attachments

(3 files, 1 obsolete file)

Attached image gmaps-error.jpg (deleted) —
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.
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
Hardware acceleration is enable.
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
Component: Untriaged → JavaScript Engine
Keywords: regression
Product: Firefox → Core
Version: Trunk → 26 Branch
FYI, Setting javascript.options.ion.content;false fixes the problem.
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
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.
Attached patch Temporary fix (obsolete) (deleted) — Splinter Review
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 on attachment 803437 [details] [diff] [review] Temporary fix Review of attachment 803437 [details] [diff] [review]: ----------------------------------------------------------------- r=me
Attachment #803437 - Flags: review?(terrence) → review+
Attached patch Cleaner fix (deleted) — Splinter Review
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)
Alice, do you know if the lack of textures when zooming out is also due to the same cset?
Flags: needinfo?(alice0775)
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+
Whiteboard: [leave open]
(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 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 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+
Also added a comment for explaining why the check makes sense. https://hg.mozilla.org/integration/mozilla-inbound/rev/13568a3576cd
Whiteboard: [leave open]
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
Depends on: 916816
Should this be considered fixed with Bug 916816 describing the exact same scenario?
No longer depends on: 916816
(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.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: