David Mandelin [:dmandelin] Assignee
	Description • 15 years ago

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.

	David Mandelin [:dmandelin] 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.

	David Mandelin [:dmandelin] 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.

	David Mandelin [:dmandelin] 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.

	Boris Zbarsky [:bzbarsky]
	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.

	David Mandelin [:dmandelin] 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.

	David Mandelin [:dmandelin] 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.

	David Anderson [:dvander] - inactive, e-mail if emergency
	Comment 7 • 15 years ago

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.

	David Mandelin [:dmandelin] Assignee
	Comment 8 • 15 years ago

Pushed to TM as 238e8b557e4f.

	David Mandelin [:dmandelin] 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.

	David Mandelin [:dmandelin] Assignee
	Comment 10 • 15 years ago

This goes on top of the patch for bug 510642. Running on try server now.

	Robert Sayre
	Comment 13 • 15 years ago

http://hg.mozilla.org/mozilla-central/rev/238e8b557e4f

	David Mandelin [:dmandelin] 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

	Robert Sayre
	Comment 15 • 15 years ago

ah, ok, mozilla-central has the backouts too. sorry.

	David Mandelin [:dmandelin] Assignee
	Comment 16 • 15 years ago

Patch 7 pushed to TM as 1f50f79db6e7.

	Bob Clary [:bc] (inactive)
	Comment 19 • 15 years ago

TraceRecorder::scopeChainProp crash covered by browser/jit js1_5/Regress/regress-470758-02.js

	Blake Kaplan (:mrbkap) (inactive)
	Comment 20 • 15 years ago

http://hg.mozilla.org/mozilla-central/rev/1f50f79db6e7

	Blake Kaplan (:mrbkap) (inactive)
	Comment 21 • 15 years ago

http://hg.mozilla.org/mozilla-central/rev/3fd6082b6a1d

	David Mandelin [:dmandelin] Assignee
	Comment 23 • 15 years ago

Nominating blocking1.9.2? because this causes a crash on Quantas (see bug 516203).

	Carsten Book [:Tomcat]
	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

	Robert Sayre
	Comment 25 • 15 years ago

http://hg.mozilla.org/releases/mozilla-1.9.2/rev/d1d22d301649