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)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla1.9.3a5
People
(Reporter: bzbarsky, Assigned: brendan)
References
()
Details
(Whiteboard: fixed-in-tracemonkey)
Attachments
(1 file, 2 obsolete files)
(deleted),
patch
|
brendan
:
review+
|
Details | Diff | Splinter Review |
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.
Reporter | ||
Comment 1•15 years ago
|
||
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.
Comment 2•14 years ago
|
||
Engadget just described us as "mega slow" because we're slow on this demo, sigh.
Assignee | ||
Comment 3•14 years ago
|
||
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
Assignee | ||
Comment 4•14 years ago
|
||
Attachment #448669 -
Flags: review?(gal)
Assignee | ||
Comment 5•14 years ago
|
||
Attachment #448669 -
Attachment is obsolete: true
Attachment #448672 -
Flags: review?(gal)
Attachment #448669 -
Flags: review?(gal)
Updated•14 years ago
|
Attachment #448672 -
Flags: review?(gal) → review+
Assignee | ||
Comment 6•14 years ago
|
||
Attachment #448672 -
Attachment is obsolete: true
Attachment #448674 -
Flags: review+
Assignee | ||
Comment 7•14 years ago
|
||
Whiteboard: fixed-in-tracemonkey
Reporter | ||
Comment 8•14 years ago
|
||
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?
Assignee | ||
Comment 9•14 years ago
|
||
I'm optimizing, will split bugs and make a metabug on optimization fault ;-).
/be
Assignee | ||
Comment 10•14 years ago
|
||
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
Comment 11•14 years ago
|
||
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
...
Comment 12•14 years ago
|
||
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.
Assignee | ||
Comment 13•14 years ago
|
||
(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
Comment 14•14 years ago
|
||
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.
Comment 15•14 years ago
|
||
Here on Windows 7 (with D2D on) it's still pretty damn chunky compared to Chrome.
Comment 16•14 years ago
|
||
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Reporter | ||
Comment 17•14 years ago
|
||
Did anyone actually measure? Do we trace this now? I don't think anyone answered my question in comment 8.
Assignee | ||
Comment 18•14 years ago
|
||
(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
Reporter | ||
Comment 19•14 years ago
|
||
> 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.
Assignee | ||
Comment 20•14 years ago
|
||
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.
Description
•