Closed Bug 523452 Opened 15 years ago Closed 14 years ago

"depth of field" chrome experiment doesn't trace

Categories

(Core :: JavaScript Engine, defect, P1)

defect

Tracking

()

RESOLVED FIXED
mozilla1.9.3a5

People

(Reporter: bzbarsky, Assigned: brendan)

References

()

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(1 file, 2 obsolete files)

jitstats: recorder: started(131), aborted(74), completed(537), different header(0), trees trashed(1), slot promoted(0), unstable loop variable(148), breaks(2), returns(0), merged loop exits(59), unstableInnerCalls(7), blacklisted(8), unblacklisted(0) monitor: exits(2130702), timeouts(13), type mismatch(0), triggered(2130715), global mismatch(6), flushed(6) aborts (with counts, gotten via "grep "Abort recording" ~/log.txt | grep chromeexper | sort | uniq -c | sort -n"; I don't know why the counts don't match the jitstats, but it worries me): 1 Abort recording of tree http://mrdoob.com/projects/chromeexperiments/depth_of_field/js/JSTweener.js:98@22 at http://mrdoob.com/projects/chromeexperiments/depth_of_field/js/JSTweener.js:120@248: No compatible inner tree. 1 Abort recording of tree http://mrdoob.com/projects/chromeexperiments/depth_of_field/js/Main.js:56@155 at http://mrdoob.com/projects/chromeexperiments/depth_of_field/js/Main.js:58@195: Inner tree is trying to grow, abort outer recording. 1 Abort recording of tree http://mrdoob.com/projects/chromeexperiments/depth_of_field/js/Main.js:73@335 at http://mrdoob.com/projects/chromeexperiments/depth_of_field/js/Main.js:75@378: Inner tree is trying to grow, abort outer recording. 1 Abort recording of tree http://mrdoob.com/projects/chromeexperiments/depth_of_field/js/Main.js:77@424 at http://mrdoob.com/projects/chromeexperiments/depth_of_field/js/Main.js:79@468: Inner tree is trying to grow, abort outer recording. 3 Abort recording of tree http://mrdoob.com/projects/chromeexperiments/depth_of_field/js/Main.js:152@24 at http://mrdoob.com/projects/chromeexperiments/depth_of_field/js/JSTweener.js:63@105: No compatible inner tree. 3 Abort recording of tree http://mrdoob.com/projects/chromeexperiments/depth_of_field/js/Main.js:176@148 at http://mrdoob.com/projects/chromeexperiments/depth_of_field/js/Main.js:182@212: name. 6 Abort recording of tree http://mrdoob.com/projects/chromeexperiments/depth_of_field/js/JSTweener.js:63@105 at http://mrdoob.com/projects/chromeexperiments/depth_of_field/js/JSTweener.js:66@145: delelem. 10 Abort recording of tree http://mrdoob.com/projects/chromeexperiments/depth_of_field/js/JSTweener.js:98@22 at http://mrdoob.com/projects/chromeexperiments/depth_of_field/js/JSTweener.js:122@268: Inner tree is trying to grow, abort outer recording. 31 Abort recording of tree http://mrdoob.com/projects/chromeexperiments/depth_of_field/js/JSTweener.js:98@22 at http://mrdoob.com/projects/chromeexperiments/depth_of_field/js/JSTweener.js:121@252: Inner tree is trying to grow, abort outer recording.
OK, the delelem abort is on this code: for (var key in this.defaultOptions) { if (typeof options[key] != 'undefined') { o[key] = options[key]; delete options[key]; } else { o[key] = this.defaultOptions[key]; } } The |name| abort is on this line: if (focus + particle.sz < 0) where we have at toplevel |var focus = camera.focus|, and right before that |camera.focus = 200|... The actual abort looks like this: trace stopped: 3643: non-stub getter trace stopped: 12359: name() not accessing a valid slot Abort recording of tree http://mrdoob.com/projects/chromeexperiments/depth_of_field/js/Main.js:176@148 at http://mrdoob.com/projects/chromeexperiments/depth_of_field/js/Main.js:182@212: name.
Blocks: 523298
Engadget just described us as "mega slow" because we're slow on this demo, sigh.
Can't be "mega slow", must fix. /be
Assignee: general → brendan
Status: NEW → ASSIGNED
OS: Mac OS X → All
Priority: -- → P1
Hardware: x86 → All
Target Milestone: --- → mozilla1.9.3a5
Attached patch proposed fix (obsolete) (deleted) — Splinter Review
Attachment #448669 - Flags: review?(gal)
Attached patch proposed fix, v2 (obsolete) (deleted) — Splinter Review
Attachment #448669 - Attachment is obsolete: true
Attachment #448672 - Flags: review?(gal)
Attachment #448669 - Flags: review?(gal)
Attachment #448672 - Flags: review?(gal) → review+
Attached patch patch to commit, with tests (deleted) — Splinter Review
Attachment #448672 - Attachment is obsolete: true
Attachment #448674 - Flags: review+
Whiteboard: fixed-in-tracemonkey
Is this actually fixed? The fix eliminated one particular abort out of the ones I listed in comment 0; do we now stay on trace well enough here that there's no more work that needs doing on it? Or put another way, should there have been a separate bug blocking this one on delelem?
I'm optimizing, will split bugs and make a metabug on optimization fault ;-). /be
Followup fix, code review while dreaming (not recommended): http://hg.mozilla.org/tracemonkey/rev/4cac2c63f055 Also made a belated static function name cleanup that gal kind of suggested yesterday: s/js_Del/Delete/g. /be
cd js/src/tests $OBJDIR/js -j -f shell.js -f js1_5/shell.js -f js1_5/GC/shell.js /Users/jason/dev/tracemonkey/js/src/tests/js1_5/GC/regress-341877-01.js Assertion failure: status == ARECORD_COMPLETED || status == ARECORD_ABORTED || status == ARECORD_ERROR, at ../jstracer.cpp:7143 (gdb) bt #0 0x0014701f in JS_Assert (s=0x1ed924 "status == ARECORD_COMPLETED || status == ARECORD_ABORTED || status == ARECORD_ERROR", file=0x1e7091 "../jstracer.cpp", ln=7143) at ../jsutil.cpp:77 #1 0x0019b4a3 in js::TraceRecorder::monitorRecording (this=0x81fc00, op=JSOP_DELELEM) at ../jstracer.cpp:7141 ...
This works reasonably well on TM tip now. It also works ok on my old JM build. I'm going to try to make a benchmarky version.
(In reply to comment #11) > cd js/src/tests > $OBJDIR/js -j -f shell.js -f js1_5/shell.js -f js1_5/GC/shell.js > /Users/jason/dev/tracemonkey/js/src/tests/js1_5/GC/regress-341877-01.js > > Assertion failure: status == ARECORD_COMPLETED || status == ARECORD_ABORTED || > status == ARECORD_ERROR, at ../jstracer.cpp:7143 Sorry, and thanks for wiping up: http://hg.mozilla.org/tracemonkey/rev/b2fc69447133 /be
OK, checked this again in 3.6 in Shark. In that version, this demo is slow, but we only spend 7.5% of time under js_Interpret. Most of the time is in graphics stuff. And the main gfx problem seems to have been fixed already on trunk.
Depends on: 569843
Here on Windows 7 (with D2D on) it's still pretty damn chunky compared to Chrome.
Depends on: 569849
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Did anyone actually measure? Do we trace this now? I don't think anyone answered my question in comment 8.
(In reply to comment #17) > Did anyone actually measure? Do we trace this now? I don't think anyone > answered my question in comment 8. bz: I'm lame, and about to get on a plane to LHR. If you could test, I'd be again in your debt. Otherwise I'll do it when landed and settled. /be
> Otherwise I'll do it when landed and settled. Did this ever happen? I'm doomed enough that there's no way I'll have time to test this properly.
Still on my TODO list -- I won't forget, but anyone cc'ed who can help, please do. /be
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: