Closed Bug 505591 Opened 11 years ago Closed 10 years ago

TM: Trace JSOP_NAME for returned closures

Categories

(Core :: JavaScript Engine, defect, P2)

defect

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)

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.
Depends on: 471214
Blocks: 501472
Attached patch Patch (obsolete) — Splinter Review
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?
Attachment #393582 - Flags: review? → review?(jorendorff)
Attachment #393582 - Flags: review?(brendan)
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.
Attachment #393582 - Flags: review?(jorendorff)
Attachment #393582 - Flags: review?(brendan)
Attached patch Patch 2 (WIP) (obsolete) — Splinter Review
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
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.
Attached patch Patch 3 (obsolete) — Splinter Review
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)
Attached patch Patch 4 (obsolete) — Splinter Review
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)
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+
Pushed to TM as 238e8b557e4f.
Whiteboard: fixed-in-tracemonkey
I backed this out Friday night because of talos tjss failures. I think bug 510642 was the underlying problem.
Depends on: 510642
No longer depends on: 471214
Whiteboard: fixed-in-tracemonkey
Attached patch Patch 5 (obsolete) — Splinter Review
This goes on top of the patch for bug 510642. Running on try server now.
Attachment #394320 - Attachment is obsolete: true
Blocks: 510554
Blocks: 509639
Attached patch Patch 6 (5 rebased to tip) (obsolete) — Splinter Review
Attachment #395355 - Attachment is obsolete: true
Attachment #395878 - Attachment is obsolete: true
http://hg.mozilla.org/mozilla-central/rev/238e8b557e4f
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
(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
ah, ok, mozilla-central has the backouts too. sorry.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Attachment #395881 - Flags: review?(dvander)
Attachment #395881 - Flags: review?(dvander) → review+
Patch 7 pushed to TM as 1f50f79db6e7.
Whiteboard: fixed-in-tracemonkey
Duplicate of this bug: 509639
Duplicate of this bug: 511241
TraceRecorder::scopeChainProp crash covered by browser/jit js1_5/Regress/regress-470758-02.js
http://hg.mozilla.org/mozilla-central/rev/1f50f79db6e7
Status: REOPENED → RESOLVED
Closed: 10 years ago10 years ago
Resolution: --- → FIXED
Duplicate of this bug: 516203
Nominating blocking1.9.2? because this causes a crash on Quantas (see bug 516203).
Flags: blocking1.9.2?
Flags: blocking1.9.2? → blocking1.9.2+
Priority: -- → P2
(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
Depends on: 523793
You need to log in before you can comment on or make changes to this bug.