Closed Bug 505591 Opened 11 years ago Closed 10 years ago
TM: Trace JSOP
_NAME for returned closures
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.
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)
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.
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.
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.
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 #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.
I backed this out Friday night because of talos tjss failures. I think bug 510642 was the underlying problem.
This goes on top of the patch for bug 510642. Running on try server now.
Attachment #394320 - Attachment is obsolete: true
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 <email@example.com> date: Fri Aug 14 19:17:00 2009 -0700 summary: Backed out changeset 238e8b557e4f: causing tjss orange changeset: 31570:238e8b557e4f user: David Mandelin <firstname.lastname@example.org> 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)
10 years ago
Attachment #395881 - Flags: review?(dvander) → review+
Patch 7 pushed to TM as 1f50f79db6e7.
TraceRecorder::scopeChainProp crash covered by browser/jit js1_5/Regress/regress-470758-02.js
Status: REOPENED → RESOLVED
Closed: 10 years ago → 10 years ago
Resolution: --- → FIXED
Nominating blocking1.9.2? because this causes a crash on Quantas (see bug 516203).
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
You need to log in before you can comment on or make changes to this bug.