Closed
Bug 505591
Opened 15 years ago
Closed 15 years ago
TM: Trace JSOP_NAME for returned closures
Categories
(Core :: JavaScript Engine, defect, P2)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
status1.9.2 | --- | beta1-fixed |
People
(Reporter: dmandelin, Assigned: dmandelin)
References
Details
(Whiteboard: fixed-in-tracemonkey)
Attachments
(1 file, 6 obsolete files)
(deleted),
patch
|
dvander
:
review+
|
Details | Diff | Splinter Review |
Bug 496240 traces JSOP_NAME for active closures. This is a followon to trace it for returned closures. See also bug 496240 comment 8 for some implementation considerations. My plan is to refactor so that js_GetPropertyHelper doesn't need to read the interpreter pc.
Assignee | ||
Comment 1•15 years ago
|
||
The idea of the fix is pretty simple. There are two things I'm not sure about, and that perhaps reviewers can confirm or refute: 1. js_GetPropertyById could potentially call JS_BytecodeFromPC if the property is not found. I haven't seen any test cases where it happens, though. I know that the original trace will not be recorded in this case, because the not-found property will cause an abort. I tried to create test cases where the property will be not found on trace by using |delete| between trace runs. But I never found any bug that way. It appears that properties created by variable declarations are generally |DontDelete| (in ECMA-262), with the possible exception of frames created by calling |eval|. But I didn't find a bug when the closure is created inside an eval, either. 2. js_GetPropertyById could potentially return JS_FALSE, but similarly to #1, it seems like that can't happen. I do assert for that condition.
Attachment #393582 -
Flags: review?
Assignee | ||
Updated•15 years ago
|
Attachment #393582 -
Flags: review? → review?(jorendorff)
Assignee | ||
Updated•15 years ago
|
Attachment #393582 -
Flags: review?(brendan)
Assignee | ||
Comment 2•15 years ago
|
||
bz just gave me a test case where this doesn't trace a returned closure if the closure is returned before the tracing occurs. I'm working on fixing this. The questions in comment 1 still apply but other parts of the patch will change.
Assignee | ||
Updated•15 years ago
|
Attachment #393582 -
Flags: review?(jorendorff)
Attachment #393582 -
Flags: review?(brendan)
Assignee | ||
Comment 3•15 years ago
|
||
This version can trace the situation described in the previous comment. It's a WIP because it uses a cheat to make js_InferFlags not crash. Note that even with this, the charAt example in the other bug doesn't speed up. This is because the out-of-range charAt calls cause us to exit trace with an OOM_EXIT.
Attachment #393582 -
Attachment is obsolete: true
![]() |
||
Comment 4•15 years ago
|
||
Hmm. I don't see the OOM_EXIT on the shell charAt testcase. And I do get a 2x speedup on the actual dromaeo charAt with this last patch. I also get a bump from 12fps to about 16fps in the bug 508716 testcase.
Assignee | ||
Comment 5•15 years ago
|
||
This patch got test failures on try server for mochitest-chrome on MacOS and Windows, with no failures on Linux. I reran mochitest-chrome locally with no problem, so I'm not sure if this is a spurious error or what.
Attachment #393648 -
Attachment is obsolete: true
Attachment #394082 -
Flags: review?(gal)
Assignee | ||
Comment 6•15 years ago
|
||
I fixed the mochtest-chrome failure. (It appeared only in optimized builds.) I'm now seeing an xpcshell failure on try, but only Win/Linux.
Attachment #394082 -
Attachment is obsolete: true
Attachment #394082 -
Flags: review?(gal)
Assignee | ||
Updated•15 years ago
|
Attachment #394320 -
Flags: review?(gal)
Comment on attachment 394320 [details] [diff] [review] Patch 4 >+ JSScopeProperty *sprop = (JSScopeProperty*) prop; ----------------------^ silly nit, jstracer whitespace consistency >+// Parameters needed to access a value from a closure on trace. >+struct ClosureVarInfo Would prefer this live in jstracer.cpp with forward declaration outside - to avoid rebuilding the whole engine on changes. Up to you though.
Attachment #394320 -
Flags: review?(gal) → review+
Assignee | ||
Comment 9•15 years ago
|
||
I backed this out Friday night because of talos tjss failures. I think bug 510642 was the underlying problem.
Updated•15 years ago
|
Whiteboard: fixed-in-tracemonkey
Assignee | ||
Comment 10•15 years ago
|
||
This goes on top of the patch for bug 510642. Running on try server now.
Attachment #394320 -
Attachment is obsolete: true
Assignee | ||
Comment 11•15 years ago
|
||
Attachment #395355 -
Attachment is obsolete: true
Assignee | ||
Comment 12•15 years ago
|
||
Attachment #395878 -
Attachment is obsolete: true
Comment 13•15 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/238e8b557e4f
Status: NEW → RESOLVED
Closed: 15 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 14•15 years ago
|
||
(In reply to comment #13) > http://hg.mozilla.org/mozilla-central/rev/238e8b557e4f That changeset was backed out from TM. Patch 7 is waiting to land following bug 510642, which caused the tjss orange. changeset: 31572:72a50720cd40 user: David Mandelin <dmandelin@mozilla.com> date: Fri Aug 14 19:17:00 2009 -0700 summary: Backed out changeset 238e8b557e4f: causing tjss orange changeset: 31570:238e8b557e4f user: David Mandelin <dmandelin@mozilla.com> date: Fri Aug 14 16:02:47 2009 -0700 summary: Bug 505591: trace JSOP_NAME for returned closures, r=dvander
Comment 15•15 years ago
|
||
ah, ok, mozilla-central has the backouts too. sorry.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Assignee | ||
Updated•15 years ago
|
Attachment #395881 -
Flags: review?(dvander)
![]() |
||
Updated•15 years ago
|
Attachment #395881 -
Flags: review?(dvander) → review+
Assignee | ||
Comment 16•15 years ago
|
||
Patch 7 pushed to TM as 1f50f79db6e7.
Whiteboard: fixed-in-tracemonkey
Comment 19•15 years ago
|
||
TraceRecorder::scopeChainProp crash covered by browser/jit js1_5/Regress/regress-470758-02.js
Comment 20•15 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/1f50f79db6e7
Status: REOPENED → RESOLVED
Closed: 15 years ago → 15 years ago
Resolution: --- → FIXED
Comment 21•15 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/3fd6082b6a1d
Assignee | ||
Comment 23•15 years ago
|
||
Nominating blocking1.9.2? because this causes a crash on Quantas (see bug 516203).
Flags: blocking1.9.2?
Updated•15 years ago
|
Flags: blocking1.9.2? → blocking1.9.2+
Priority: -- → P2
Comment 24•15 years ago
|
||
(In reply to comment #23) > Nominating blocking1.9.2? because this causes a crash on Quantas (see bug > 516203). note - also a testcase is searching for flights on lufthansa.com/de -> http://crash-stats.mozilla.com/report/index/bp-f5ba17d8-c2ad-415c-9b7d-f165f2090924
Comment 25•15 years ago
|
||
http://hg.mozilla.org/releases/mozilla-1.9.2/rev/d1d22d301649
status1.9.2:
--- → beta1-fixed
You need to log in
before you can comment on or make changes to this bug.
Description
•