505591 - TM: Trace JSOP_NAME for returned closures

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P2)

Description

[David Mandelin [:dmandelin]]

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.

Comment 1

[David Mandelin [:dmandelin]]

Attachment: Patch

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.

Comment 2

[David Mandelin [:dmandelin]]

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.

Comment 3

[David Mandelin [:dmandelin]]

Attachment: Patch 2 (WIP)

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.

Comment 4

[Boris Zbarsky [:bzbarsky, bz on IRC]]

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.

Comment 5

[David Mandelin [:dmandelin]]

Attachment: Patch 3

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.

Comment 6

[David Mandelin [:dmandelin]]

Attachment: Patch 4

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.

Comment 7

[David Anderson [:dvander] - inactive, e-mail if emergency]

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.

Comment 8

[David Mandelin [:dmandelin]]

Pushed to TM as 238e8b557e4f.

Comment 9

[David Mandelin [:dmandelin]]

I backed this out Friday night because of talos tjss failures. I think bug 510642 was the underlying problem.

Comment 10

[David Mandelin [:dmandelin]]

Attachment: Patch 5

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

Comment 11

[David Mandelin [:dmandelin]]

Attachment: Patch 6 (5 rebased to tip)

Comment 12

[David Mandelin [:dmandelin]]

Attachment: Patch 7 (corrects a typo in 6)

Comment 13

[Robert Sayre]

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

Comment 14

[David Mandelin [:dmandelin]]

(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 <[e-mail]>
date:        Fri Aug 14 19:17:00 2009 -0700
summary:     Backed out changeset 238e8b557e4f: causing tjss orange

changeset:   31570:238e8b557e4f
user:        David Mandelin <[e-mail]>
date:        Fri Aug 14 16:02:47 2009 -0700
summary:     Bug 505591: trace JSOP_NAME for returned closures, r=dvander

Comment 15

[Robert Sayre]

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

Comment 16

[David Mandelin [:dmandelin]]

Patch 7 pushed to TM as 1f50f79db6e7.

Comment 19

[Bob Clary [:bc:]]

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

Comment 20

[Blake Kaplan (:mrbkap) (inactive, use needinfo)]

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

Comment 21

[Blake Kaplan (:mrbkap) (inactive, use needinfo)]

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

Comment 23

[David Mandelin [:dmandelin]]

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

Comment 24

[Carsten Book [:Tomcat]]

(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

[Robert Sayre]

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