Last Comment Bug 554955 - Property cache bug walking the scope chain across a heavyweight frame (involving JSOP_NAME, teleport, eval)
: Property cache bug walking the scope chain across a heavyweight frame (involv...
Status: RESOLVED FIXED
softblocker, fixed-in-tracemonkey
: regression, testcase
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Other Branch
: All All
: -- normal (vote)
: ---
Assigned To: Jim Blandy :jimb
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on: 643222 646366
Blocks: 497789 619750
  Show dependency treegraph
 
Reported: 2010-03-25 08:21 PDT by Jason Orendorff [:jorendorff]
Modified: 2011-03-31 14:29 PDT (History)
12 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
.x+


Attachments
Widen AssertValidPropertyCacheHit to detect this bug. (476 bytes, patch)
2010-11-15 17:55 PST, Jim Blandy :jimb
no flags Details | Diff | Splinter Review
Introduce a typed accessor for retreiving functions from JSFunctionBoxes. (3.70 KB, patch)
2010-12-06 17:31 PST, Jim Blandy :jimb
brendan: review+
Details | Diff | Splinter Review
Give blocks and call objects unique shapes when they have parents that may be extended with new bindings. (16.07 KB, patch)
2010-12-06 17:32 PST, Jim Blandy :jimb
no flags Details | Diff | Splinter Review
Give blocks and call objects unique shapes when they have parents that may be extended with new bindings. (17.51 KB, patch)
2010-12-16 11:19 PST, Jim Blandy :jimb
jorendorff: review+
Details | Diff | Splinter Review
Make JSObject::setMap not pretend to take a const shape. (2.01 KB, patch)
2011-01-21 23:16 PST, Jim Blandy :jimb
jorendorff: review+
Details | Diff | Splinter Review
Give blocks and call objects unique shapes when they have parents that may be extended with new bindings. (18.71 KB, patch)
2011-01-21 23:16 PST, Jim Blandy :jimb
jorendorff: review+
Details | Diff | Splinter Review
Give blocks and call objects unique shapes when they have parents that may be extended with new bindings. (22.96 KB, patch)
2011-02-22 16:52 PST, Jim Blandy :jimb
no flags Details | Diff | Splinter Review
Give blocks and call objects unique shapes when they have parents that may be extended with new bindings. (23.10 KB, patch)
2011-02-23 13:49 PST, Jim Blandy :jimb
jorendorff: review+
Details | Diff | Splinter Review
Revert test and assertion changes from bug 633890. r=jorendorff (1.67 KB, patch)
2011-03-15 12:18 PDT, Jim Blandy :jimb
no flags Details | Diff | Splinter Review

Description Jason Orendorff [:jorendorff] 2010-03-25 08:21:22 PDT
function f(s) {
    eval(s);
    return function (a) {
        var d;
        let (c = 3) {
            d = function() { a; }; // force Block object to be cloned
            return b; // lookup occurs in scope of Block
        }
    };
}

var b = 1;
var g1 = f("");
var g2 = f("var b = 2;");
g1(0);  // call the lambda once, cache reference to global b
assertEq(g2(0), 2);  // call the lambda again, erroneous property cache hit

I get "Error: Assertion failed: got 1, expected 2".

Two objects with the same shape can have different scope chains.

I think this bug is a regression due to the change in bug 497789 away from (obj, atom)-keyed property cache entries. I missed it during review. :-P
Comment 1 Jason Orendorff [:jorendorff] 2010-03-25 11:37:32 PDT
The two Block objects have the same shape because they have the same proto. I think bug 554793 prevents this from being a problem for Call objects.
Comment 2 Jason Orendorff [:jorendorff] 2010-03-25 13:00:44 PDT
Proposed fix:

New shape guarantee - The shape of a CacheableNonGlobalScope covers the shapes of all its parents up to, but not including, the global object.

This guarantee would close the hole. And it's almost true already. The only problem is that eval can declare new vars in Call objects. To fix this, when we create a new CacheableNonGlobalScope, if any parent is a Call object that's subject to direct eval, we need to give the new object a unique shape.

There are three CacheableNonGlobalScope classes: Call, Block, and DeclEnv. For Call and Block, we can set a bit in the compile-time model to mark scopes requiring unique shapes. For DeclEnv, we can either set aside a bit in  JSFunction or walk the scope chain.

Nothing special needs to happen on trace, since the fix only applies at create time, and we don't create these objects on trace.
Comment 3 Brendan Eich [:brendan] 2010-03-25 13:37:20 PDT
See bug 514568, not yet fixed. If the direct eval is in an es5-strict-mode code body (top level or function), it cannot create vars in that code body's scope. So we don't need to deoptimize if these stars align.

/be
Comment 4 Damon Sicore (:damons) 2010-07-29 16:44:23 PDT
Can we get an owner here, please?
Comment 5 Jim Blandy :jimb 2010-11-10 15:35:56 PST
Looking into this.
Comment 6 Jim Blandy :jimb 2010-11-11 09:32:15 PST
Reproduced on tip.
Comment 7 Jim Blandy :jimb 2010-11-15 17:55:58 PST
Created attachment 490769 [details] [diff] [review]
Widen AssertValidPropertyCacheHit to detect this bug.

It seems to me that AssertValidPropertyCacheHit ought to be catching exactly these sorts of bugs in debug builds. It's prevented from doing so by a condition explicitly written into the function that I don't understand; removing it introduces no new regressions in js/src/tests run with all jits disabled.

This patch removes that condition. Not r?'ing yet, since I'll probably learn more later.
Comment 8 Jim Blandy :jimb 2010-11-15 21:39:09 PST
A simplified test case --- only two call objects involved. With the attached patch applied, this triggers a C++ assertion.

function f(s) {
    eval(s);
    return function(a) {
        eval(a);
        return b;
    };
}

var b = 1;
var g1 = f("");
var g2 = f("var b = 2;");
g1('');  // call the lambda once, cache reference to global b
assertEq(g2(''), 2);  // call the lambda again, erroneous property cache hit
Comment 9 Jim Blandy :jimb 2010-11-15 22:30:15 PST
I've got a bunch of other test cases, too(In reply to comment #2)
> New shape guarantee - The shape of a CacheableNonGlobalScope covers the shapes
> of all its parents up to, but not including, the global object.

As I understand it, this isn't really a new shape guarantee; we're just resuscitating the scope chain shadowing guarantee, which simply can't function if we're not indexing the cache with something that uniquely identifies the specific scope chain of objects we're doing lookups along. The object identity satisfied this requirement; shapes do not (at present).

> To fix this, when we
> create a new CacheableNonGlobalScope, if any parent is a Call object that's
> subject to direct eval, we need to give the new object a unique shape.
> 
> There are three CacheableNonGlobalScope classes: Call, Block, and DeclEnv. For
> Call and Block, we can set a bit in the compile-time model to mark scopes
> requiring unique shapes. For DeclEnv, we can either set aside a bit in 
> JSFunction or walk the scope chain.

This sounds great.  I can walk the function box tree, and use skipmin values and the depth of the closest enclosing TCF_FUN_CALLS_EVAL function to decide which functions to mark.

Is eval the only way to cause these problems? If so, then TCF_FUN_CALLS_EVAL seems like it should be enough. Or are there other ways to add bindings to Call objects?
Comment 10 Brendan Eich [:brendan] 2010-11-16 00:25:02 PST
Reminder: comment 3.

Function sub-statements can also extend Call objects at runtime, but these should be verboten in ES5 strict (bug 609832 -- see also post-fx4 bug 585536).

/be
Comment 11 Jim Blandy :jimb 2010-11-16 15:01:48 PST
(In reply to comment #9)
> This sounds great.  I can walk the function box tree, and use skipmin values
> and the depth of the closest enclosing TCF_FUN_CALLS_EVAL function to decide
> which functions to mark.

Actually, what I need is a skipmax value. :(

(In reply to comment #10)
> Reminder: comment 3.

Duly noted. Strict mode functions will not need to be uniquified harder.

> Function sub-statements can also extend Call objects at runtime, but these
> should be verboten in ES5 strict (bug 609832 -- see also post-fx4 bug 585536).

Thanks --- I had forgotten about those.
Comment 12 Jason Orendorff [:jorendorff] 2010-11-19 12:50:35 PST
Jim and I discussed comment 9 on IRC.

The scope chain shadowing guarantee, as currently documented ("Informally: if another variable or property shadows [an existing property of a scope object x'], the shape of x' will change.") is correctly implemented.

However, that guarantee isn't strong enough to support what the property cache is trying to do in the test case. So we must either pare down the cache to work under the existing shape guarantees; or shore up the shape guarantees (as comment 2 proposes) to support the existing cache.

Looks like the latter, so far.
Comment 13 Jim Blandy :jimb 2010-12-03 21:49:56 PST
I have a patch for this, in testing.
Comment 14 Jim Blandy :jimb 2010-12-06 17:31:37 PST
Created attachment 495715 [details] [diff] [review]
Introduce a typed accessor for retreiving functions from JSFunctionBoxes.
Comment 15 Jim Blandy :jimb 2010-12-06 17:32:20 PST
Created attachment 495716 [details] [diff] [review]
Give blocks and call objects unique shapes when they have parents that may be extended with new bindings.

The comments for JSObject::extensibleParents should explain why this is necessary.

AssertValidPropertyCacheHit should have been catching this bug, but for
reasons I don't understand, it is restricted from checking this case. This
patch extends it to assert when the bug is detected.
Comment 16 Jim Blandy :jimb 2010-12-06 17:33:29 PST
Sunspider shows no performance impact. This should simply be restoring the cache behavior we used to have when scope chain searches used object identities as cache keys.
Comment 17 Brendan Eich [:brendan] 2010-12-06 17:48:47 PST
Comment on attachment 495715 [details] [diff] [review]
Introduce a typed accessor for retreiving functions from JSFunctionBoxes.

Stealing, I doubt jorendorff will mind. Death to old C code! We could have used a macro, even in C -- Igor may recall why not, but no matter. r=me.

/be
Comment 18 Jim Blandy (bug-watching account --- use jimb@mozilla.com instead) 2010-12-06 18:03:14 PST
Brendan has pointed out some nits, but also suggested that we simply use OWN_SHAPE for Blocks and Calls; then, we only need a new bit in JSFunction::flags.
Comment 19 Brendan Eich [:brendan] 2010-12-06 18:15:16 PST
Also need to get to the bottom of any function-kinding bugs, with right order of calls in analyzeFunctions restored.

/be
Comment 20 Jim Blandy :jimb 2010-12-09 20:26:07 PST
(In reply to comment #19)
> Also need to get to the bottom of any function-kinding bugs, with right order
> of calls in analyzeFunctions restored.

The problem I'm aware of has been filed as bug 617609.
Comment 21 Jim Blandy :jimb 2010-12-10 16:30:31 PST
Comment on attachment 495716 [details] [diff] [review]
Give blocks and call objects unique shapes when they have parents that may be extended with new bindings.

Removing r?; Brendan's requested simplifications on the way shortly.
Comment 22 Jim Blandy :jimb 2010-12-16 11:19:39 PST
Created attachment 498164 [details] [diff] [review]
Give blocks and call objects unique shapes when they have parents that may be extended with new bindings.

[Brendan suggested keeping the function flag, but simply using OWN_SHAPE for blocks.]

The comments for JSFunction::extensibleParents explain why this is necessary.

AssertValidPropertyCacheHit should have been catching this bug, but for
reasons I don't understand, it is restricted from checking this case. This
patch extends it to assert when the bug is detected.

I've gathered the infallible parts of the initialization for Call objects
and cloned block objects into their own functions.
Comment 23 christian 2011-01-04 15:47:39 PST
As per today's meeting, beta 9 will be a time-based release. Marking these all betaN+. Please move it back to beta9+ if you believe it MUST be in the next beta (ie: trunk is in an unshippable state without this)
Comment 24 christian 2011-01-04 18:14:45 PST
Fixing fields my automated script accidentally blanked. Apologies for the bugspam
Comment 25 Jason Orendorff [:jorendorff] 2011-01-07 13:44:20 PST
Comment on attachment 498164 [details] [diff] [review]
Give blocks and call objects unique shapes when they have parents that may be extended with new bindings.

In jsinterp.cpp, AssertValidPropertyCacheHit:
>-    if (cx->runtime->gcNumber != sample || entry->vshape() != pobj->shape())
>+    if (cx->runtime->gcNumber != sample)
>         return true;

Good change. Thanks.

In jsobjinlines.h, JSObject::initCall:
>+    else
>+        objShape = map->shape;
>+
>+
>+}

Extra blank lines here.

In initClonedBlock:
>+    init(cx, &js_BlockClass, proto, NULL, (void *) frame, false);

The cast isn't necessary, is it?

In jsparse.cpp:
>+void
>+Parser::markExtensibleScopeDescendants(JSFunctionBox *funbox, bool hasExtensibleParent) {
>+    JS_ASSERT(funbox);
>+    do {
>+        JSFunction *fun = funbox->function();
[...]
>+    } while ((funbox = funbox->siblings) != NULL);

The assertion is unnecessary.

Actually I would write this loop
    for (; funbox; funbox = funbox->siblings)
which seems more idiomatic to me. It doesn't really matter.

>+         * It would be nice to use FUN_KIND(fun) here to recognize functions that will
>+         * never consult their parent chains, and thus don't need their 'extensible
>+         * parents' flag set. However, that information doesn't seem reliable at the
>+         * moment; bug <to-be-filed>.

Still to-be-filed, or do you mean bug 617609?

>+        if (funbox->kids) {
>+            /* No need to check if we already know. */
>+            if (!hasExtensibleParent) {
>+                /*
>+                 * Must this function's descendants be marked as having an extensible
>+                 * ancestor? Direct eval calls can add bindings --- although not in strict
>+                 * mode functions. Function statements also extend call objects (we will
>+                 * forbid those in strict mode code soon).
>+                 */
>+                if (((funbox->tcflags & TCF_FUN_CALLS_EVAL) &&
>+                     !(funbox->tcflags & TCF_STRICT_MODE_CODE)) ||
>+                    (funbox->tcflags & TCF_HAS_FUNCTION_STMT)) {
>+                    hasExtensibleParent = true;
>+                }
>+            }
>+
>+            markExtensibleScopeDescendants(funbox->kids, hasExtensibleParent);
>+        }

Assigning to hasExtensibleParent will affect funbox's siblings in later loop
iterations. Instead, maybe

    markExtensibleScopeDescendants(funbox->kids,
        hasExtensibleParent || funbox->scopeIsExtensible());

...but a local boolean is fine too.

Looks good. r=me.
Comment 26 Jim Blandy :jimb 2011-01-07 23:17:19 PST
Bug 514568 seems to have bitrotted this patch a bit. In particular, we pass a Bindings to NewCallObject now, instead of a function, so NewCallObject needs a different way to be told whether the Call object needs a unique shape. I'll tackle this this weekend, or Monday.
Comment 27 Jim Blandy :jimb 2011-01-07 23:26:42 PST
(In reply to comment #25)
> In jsobjinlines.h, JSObject::initCall:
> >+    else
> >+        objShape = map->shape;
> >+
> >+
> >+}
> 
> Extra blank lines here.

D'oh.

> In initClonedBlock:
> >+    init(cx, &js_BlockClass, proto, NULL, (void *) frame, false);
> 
> The cast isn't necessary, is it?

I don't think so, no.

> In jsparse.cpp:
> >+void
> >+Parser::markExtensibleScopeDescendants(JSFunctionBox *funbox, bool hasExtensibleParent) {
> >+    JS_ASSERT(funbox);
> >+    do {
> >+        JSFunction *fun = funbox->function();
> [...]
> >+    } while ((funbox = funbox->siblings) != NULL);
> 
> The assertion is unnecessary.

Okay.

> Actually I would write this loop
>     for (; funbox; funbox = funbox->siblings)
> which seems more idiomatic to me. It doesn't really matter.

Sure.

> >+         * It would be nice to use FUN_KIND(fun) here to recognize functions that will
> >+         * never consult their parent chains, and thus don't need their 'extensible
> >+         * parents' flag set. However, that information doesn't seem reliable at the
> >+         * moment; bug <to-be-filed>.
> 
> Still to-be-filed, or do you mean bug 617609?

It's bug 617609 --- thanks.

> >+        if (funbox->kids) {
> >+            /* No need to check if we already know. */
> >+            if (!hasExtensibleParent) {
> >+                /*
> >+                 * Must this function's descendants be marked as having an extensible
> >+                 * ancestor? Direct eval calls can add bindings --- although not in strict
> >+                 * mode functions. Function statements also extend call objects (we will
> >+                 * forbid those in strict mode code soon).
> >+                 */
> >+                if (((funbox->tcflags & TCF_FUN_CALLS_EVAL) &&
> >+                     !(funbox->tcflags & TCF_STRICT_MODE_CODE)) ||
> >+                    (funbox->tcflags & TCF_HAS_FUNCTION_STMT)) {
> >+                    hasExtensibleParent = true;
> >+                }
> >+            }
> >+
> >+            markExtensibleScopeDescendants(funbox->kids, hasExtensibleParent);
> >+        }
> 
> Assigning to hasExtensibleParent will affect funbox's siblings in later loop
> iterations.

Oh, how inept. Thank you very much!

I'll address these points as I do the refresh.
Comment 28 Jim Blandy :jimb 2011-01-21 23:16:10 PST
Created attachment 506071 [details] [diff] [review]
Make JSObject::setMap not pretend to take a const shape.

Without this patch, JSObject::setMap takes a const js::Shape *, which is
kind of misleading. The object will not actually treat the shape as const
(it does a const_cast). At all but one call site, setMap's argument is not
a const shape --- and in fact, many call sites are installing empty shapes
whose role in life is to be extended.

The only place where a const js::Shape appears is in setSharedNonNativeMap,
which is doing something unusual. The const cast should be there.

Changing js::Bindings::lastShape to return a non-const shape is then
natural, and cleans up the definition of initCall coming in the next patch.
Comment 29 Jim Blandy :jimb 2011-01-21 23:16:55 PST
Created attachment 506072 [details] [diff] [review]
Give blocks and call objects unique shapes when they have parents that may be extended with new bindings.

The comments for js::Bindings::extensibleParents explain why this is necessary.

AssertValidPropertyCacheHit should have been catching this bug, but for
reasons I don't understand, it is restricted from checking this case. This
patch extends it to assert when the bug is detected.

I've gathered the infallible parts of the initialization for Call objects
and cloned block objects into their own functions.
Comment 30 Jim Blandy :jimb 2011-01-21 23:17:57 PST
Comment on attachment 506072 [details] [diff] [review]
Give blocks and call objects unique shapes when they have parents that may be extended with new bindings.

Asking for a re-review on this, as adapting it to Waldo's new js::Bindings arrangement has changed things a bit.
Comment 31 Brendan Eich [:brendan] 2011-01-28 11:13:50 PST
Comment on attachment 506072 [details] [diff] [review]
Give blocks and call objects unique shapes when they have parents that may be extended with new bindings.

>+Parser::markExtensibleScopeDescendants(JSFunctionBox *funbox, bool hasExtensibleParent) 
>+{
>+    for (; funbox; funbox = funbox->siblings) {
>+        /*
>+         * It would be nice to use FUN_KIND(fun) here to recognize functions
>+         * that will never consult their parent chains, and thus don't need
>+         * their 'extensible parents' flag set. However, that information
>+         * doesn't seem reliable at the moment (bug 617609).

Bug 617609 is fixed now.

/be
Comment 32 Jim Blandy :jimb 2011-01-28 14:58:37 PST
(In reply to comment #31)
> Bug 617609 is fixed now.

How do you feel about nominating bug 619750, then?
Comment 33 Jason Orendorff [:jorendorff] 2011-01-28 16:49:53 PST
>     * Property cache entries only record the shapes of the first and last
>     * objects along the search path, so if the search traverses more than those
>     * two objects, then those first and last shapes must determine the shapes
>     * of everything else along the path.

This is too strong a statement--the first and last shapes don't actually determine the shapes of everything else; it only covers (a) the parent and prototype links that form the comb or chain, and (b) the particular properties that you can lookup at A and find on B. Other properties can be added or changed without affecting those two shapes.

(This is picky of me, but... I think it's wise to be picky about this stuff.)

I'll finish reviewing this Saturday morning.
Comment 34 Jim Blandy :jimb 2011-01-28 17:50:21 PST
(In reply to comment #33)
> This is too strong a statement--the first and last shapes don't actually
> determine the shapes of everything else; it only covers (a) the parent and
> prototype links that form the comb or chain, and (b) the particular properties
> that you can lookup at A and find on B. Other properties can be added or
> changed without affecting those two shapes.

I don't understand this. If the shape of some object along the search path (not any particular concrete linked list, but the path taken by the identifier search) can vary without the endpoint shapes varying, then it seems to me we must have a bug, because that shape change could add the property the property cache entry tells us how to find.

I'm sorry to be such a dull thinker about this; can you give a specific example?
Comment 35 Brendan Eich [:brendan] 2011-01-29 01:50:26 PST
(In reply to comment #32)
> (In reply to comment #31)
> > Bug 617609 is fixed now.
> 
> How do you feel about nominating bug 619750, then?

Wouldn't block on that one, but citing it instead of the fixed bug 617609 (which is fixed for Firefox 4) seems best, if I'm following.

/be
Comment 36 Jason Orendorff [:jorendorff] 2011-01-29 08:51:36 PST
Comment on attachment 506072 [details] [diff] [review]
Give blocks and call objects unique shapes when they have parents that may be extended with new bindings.

Looks fine. Nicer, if anything.

I didn't re-review the tests. I assume they're unchanged.
Comment 37 Jim Blandy :jimb 2011-01-31 12:10:19 PST
http://hg.mozilla.org/tracemonkey/rev/297b1312f534

I'm happy to make follow-up changes to the comments, but it still seems to me the comment is correct.
Comment 38 Jim Blandy :jimb 2011-01-31 12:22:13 PST
(In reply to comment #35)
> Wouldn't block on that one, but citing it instead of the fixed bug 617609
> (which is fixed for Firefox 4) seems best, if I'm following.

Naturally. Done.
Comment 39 Jim Blandy :jimb 2011-01-31 16:53:34 PST
Reverted:
http://hg.mozilla.org/tracemonkey/rev/9459c4b15890

Saith Talos, the stone-slinging robot defending the shores of Crete^H^H^H^H^HMozilla:

Regression :( Dromaeo (jslib) decrease 14.1% on Linux TraceMonkey
-----------------------------------------------------------------
    Previous: avg 123.463 stddev 0.858 of 30 runs up to revision a94566343ba6
    New     : avg 106.028 stddev 0.413 of 5 runs since revision 297b1312f534
    Change  : -17.435 (14.1% / z=20.320)
    Graph   : http://mzl.la/hydsaT
Comment 40 Jim Blandy :jimb 2011-01-31 17:15:46 PST
cachegrind doesn't see this effect:

------------------------------------------------------------------
instructions executed (nb: "on-trace" may overestimate slightly)
------------------------------------------------------------------
3d-cube:
  total          83,421,247      83,422,741     -- 
  on-trace       60,469,156      60,469,156     -- 
3d-morph:
  total          36,931,843      36,931,862     -- 
  on-trace       25,259,866      25,259,866     -- 
3d-raytrace:
  total          76,364,656      76,366,738     -- 
  on-trace       49,059,156      49,059,156     -- 
access-binary-trees:
  total          26,508,175      26,508,441     -- 
  on-trace       14,174,292      14,174,292     -- 
access-fannkuch:
  total         136,284,875     136,285,002     -- 
  on-trace      130,835,403     130,835,403     -- 
access-nbody:
  total          29,315,493      29,316,159     -- 
  on-trace       17,620,959      17,620,959     -- 
access-nsieve:
  total          52,370,286      52,370,558     -- 
  on-trace       47,032,789      47,032,789     -- 
bitops-3bit-bits-in-byte:
  total           7,047,530       7,047,647     -- 
  on-trace        3,497,633       3,497,633     -- 
bitops-bits-in-byte:
  total          52,622,807      52,622,991     -- 
  on-trace       48,474,442      48,474,442     -- 
bitops-bitwise-and:
  total          16,435,386      16,435,444     -- 
  on-trace       13,207,806      13,207,806     -- 
bitops-nsieve-bits:
  total          38,862,502      38,862,745     -- 
  on-trace       34,501,393      34,501,393     -- 
controlflow-recursive:
  total          22,093,564      22,094,175     -- 
  on-trace       18,533,963      18,533,963     -- 
crypto-aes:
  total          65,826,355      65,827,860     -- 
  on-trace       44,086,153      44,086,153     -- 
crypto-md5:
  total          26,154,112      26,155,428     -- 
  on-trace       15,149,035      15,149,035     -- 
crypto-sha1:
  total          20,521,424      20,522,596     -- 
  on-trace       10,443,570      10,443,570     -- 
date-format-tofte:
  total          72,097,805      72,101,616     -- 
  on-trace       29,529,058      29,529,058     -- 
date-format-xparb:
  total          53,593,657      53,595,312     -- 
  on-trace       11,036,937      11,036,937     -- 
math-cordic:
  total          38,139,535      38,139,942     -- 
  on-trace       28,960,522      28,960,522     -- 
math-partial-sums:
  total          25,461,728      25,461,877     -- 
  on-trace        5,586,521       5,586,521     -- 
math-spectral-norm:
  total          40,887,973      40,888,327     -- 
  on-trace       36,503,096      36,503,096     -- 
regexp-dna:
  total          46,340,160      46,340,451     -- 
  on-trace       34,494,296      34,494,296     -- 
string-base64:
  total          24,748,749      24,748,953     -- 
  on-trace        9,516,764       9,516,764     -- 
string-fasta:
  total          72,187,128      72,187,310     -- 
  on-trace       43,681,535      43,681,535     -- 
string-tagcloud:
  total          88,272,662      88,271,637     -- 
  on-trace       17,993,076      17,993,076     -- 
string-unpack-code:
  total          96,272,365      96,273,275     -- 
  on-trace       13,240,666      13,240,666     -- 
string-validate-input:
  total          33,172,730      33,173,458     -- 
  on-trace        8,732,717       8,732,717     -- 
-------
all:
  total       1,281,934,747   1,281,952,545     -- 
  on-trace      771,620,804     771,620,804     --
Comment 41 Jim Blandy :jimb 2011-01-31 17:34:29 PST
I hacked out the Dromaeo tests I understand Talos to be concerned with:

------------------------------------------------------------------
instructions executed (nb: "on-trace" may overestimate slightly)
------------------------------------------------------------------
dromaeo-core-eval:
  total           3,287,291       3,287,018     -- 
  on-trace            3,349           3,349     -- 
dromaeo-object-array:
  total           3,691,194       3,690,328     -- 
  on-trace            3,540           3,540     -- 
dromaeo-object-regexp:
  total          14,467,211      14,462,273     -- 
  on-trace        1,855,037       1,855,037     -- 
dromaeo-object-string:
  total           5,090,634       5,089,236     -- 
  on-trace            4,094           4,094     -- 
-------
all:
  total          26,536,330      26,528,855     -- 
  on-trace        1,866,020       1,866,020     --
Comment 42 Jim Blandy :jimb 2011-01-31 17:35:18 PST
(In comment 41, I reversed the post-patch and pre-patch shells, so the left column shows the cycle counts with the patch applied, the reverse of the comment 40.)
Comment 43 Jim Blandy :jimb 2011-01-31 18:50:39 PST
Running Dromaeo from its web site in the browser suggests that the big hit is in eval (which makes sense):

Without patch:

Code Evaluation:
153.52runs/s

Microtests of code evaluation (eval, new Function).
Origin, Source, Tests: eval, microtest

    Normal eval: 127.75runs/s ±17.78%
    new Function: 184.50runs/s ±2.02%


With patch:

Code Evaluation:
84.32runs/s

Microtests of code evaluation (eval, new Function).
Origin, Source, Tests: eval, microtest

    Normal eval: 39.29runs/s ±15.79%
    new Function: 180.95runs/s ±3.26%
Comment 44 Chris Leary [:cdleary] (not checking bugmail) 2011-01-31 19:52:10 PST
cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/f6117465a979
http://hg.mozilla.org/mozilla-central/rev/26d40e1e80bf
http://hg.mozilla.org/mozilla-central/rev/18a1effafe19
http://hg.mozilla.org/mozilla-central/rev/297b1312f534
Note: not marking as fixed because fixed-in-tracemonkey is not present on the whiteboard.
Comment 45 Chris Leary [:cdleary] (not checking bugmail) 2011-02-06 16:57:35 PST
cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/9459c4b15890
Note: not marking as fixed because fixed-in-tracemonkey is not present on the whiteboard.
Comment 46 Jim Blandy :jimb 2011-02-17 10:14:53 PST
I've finally figured out what is causing the performance regression here. The TCF_FUN_CALLS_EVAL flag is set not only on functions that call eval themselves, but on functions that contain any nested functions that have the flag set. This means that, in code like this:

function f() {
  function g() {
    eval('');
  }
}

both f and g's bindings would get their hasExtensibleParents flags set, whereas it's only necessary to set the flag on *children* of g. Fixing that brings the Dromaeo scores back up.

There's a flaw in my current fix which the tests don't catch. I should have that revised and new patches up in an hour or so.
Comment 47 Jim Blandy :jimb 2011-02-17 23:06:19 PST
Found a distinct bug which this also fixes. Consider a function like this:

js> var b = 'global' 
js> function f(s) { eval(s); let (a=3) { eval(''); return b; } }
js> f('')
"global"
js> f("var b = 'local'")
"global"
js> 

That |eval(s)| can add bindings for b to f's call object, but since we haven't yet cloned the lexical block, the call object is not marked as a delegate, so any DEFVAR executed by the eval(s) won't do a scope chain purge.

My patch ensures that we never re-use shapes within an extensible scope, and thus the problem never arises.
Comment 48 Jim Blandy :jimb 2011-02-17 23:07:47 PST
Given that this bug shows incorrect variable lookup in pure ES3 code (see js1_8_5/regress/regress-554955-2.js, added by the patch), and has a patch, should it be a hardblocker?
Comment 49 Jim Blandy :jimb 2011-02-17 23:09:11 PST
(Accidentally removed softblocker marking. Added it back, along with nomination.)
Comment 50 Jim Blandy :jimb 2011-02-17 23:09:49 PST
Will upload new patch in the AM.
Comment 51 David Mandelin [:dmandelin] 2011-02-18 10:00:12 PST
Setting .x since softblocker now means nonblocking anyway.
Comment 52 Jason Orendorff [:jorendorff] 2011-02-22 07:15:11 PST
(In reply to comment #47)
> That |eval(s)| can add bindings for b to f's call object, but since we haven't
> yet cloned the lexical block, the call object is not marked as a delegate, so
> any DEFVAR executed by the eval(s) won't do a scope chain purge.

I haven't thought this through all the way, but yeah, that sounds sick.

Please be sure to add this as a test.

Quick sanity check: Are you sure it isn't Bad that the delegate bit isn't set? Also, is there a way to get the let-block in your new example to read a deleted var (non-strict direct eval can create deleteable vars)?

I don't think so, but these are the things I would check.
Comment 53 Jim Blandy :jimb 2011-02-22 16:52:24 PST
Created attachment 514361 [details] [diff] [review]
Give blocks and call objects unique shapes when they have parents that may be extended with new bindings.

I've changed the analysis to use a new bit that isn't inherited by enclosing functions, so I'm asking for a new review; this corrects the performance issues caught by Dromaeo.
Comment 54 Jim Blandy :jimb 2011-02-23 13:49:15 PST
Created attachment 514593 [details] [diff] [review]
Give blocks and call objects unique shapes when they have parents that may be extended with new bindings.

Comment clarifications.
Comment 55 Jim Blandy :jimb 2011-02-28 12:34:09 PST
Comment on attachment 514593 [details] [diff] [review]
Give blocks and call objects unique shapes when they have parents that may be extended with new bindings.

Asking for approval2.0: this bug causes simple variable references to find the wrong binding in ordinary ES5 code (as shown in comment 48, and the test js1_8_5/regress/regress-554955-2.js, added by the patch).
Comment 56 Jim Blandy :jimb 2011-02-28 12:37:56 PST
Sorry --- should be comment 47, and it's not ES5 code. But the test case mentioned does fit the bill:

+function f(s) {
+    eval(s);
+    return function(a) {
+        with({}) {}; // repel JägerMonkey
+        eval(a);
+        return b;
+    };
+}
+
+var b = 1;
+var g1 = f("");
+var g2 = f("var b = 2;");
+
+/* Call the lambda once, caching a reference to the global b. */
+g1('');
+
+/*
+ * If this call sees the above cache entry, then it will erroneously use
+ * the global b.
+ */
+assertEq(g2(''), 2);
Comment 57 Jim Blandy :jimb 2011-02-28 14:10:07 PST
(In reply to comment #52)
> Quick sanity check: Are you sure it isn't Bad that the delegate bit isn't set?

Yes --- at the time, the call object is indeed not a delegate, so the operation is sound; the error comes when we make it parent things with non-fresh shapes. With the patch, that never happens any more.

> Also, is there a way to get the let-block in your new example to read a deleted
> var (non-strict direct eval can create deleteable vars)?

No, because the cache entry is keyed on the shape where the variable was found.
Comment 58 Brendan Eich [:brendan] 2011-03-01 11:04:18 PST
Comment on attachment 514593 [details] [diff] [review]
Give blocks and call objects unique shapes when they have parents that may be extended with new bindings.

>diff --git a/js/src/jspropertycache.cpp b/js/src/jspropertycache.cpp
>--- a/js/src/jspropertycache.cpp
>+++ b/js/src/jspropertycache.cpp
>@@ -61,6 +61,7 @@ PropertyCache::fill(JSContext *cx, JSObj
> 
>     JS_ASSERT(this == &JS_PROPERTY_CACHE(cx));
>     JS_ASSERT(!cx->runtime->gcRunning);
>+    JS_ASSERT_IF(adding, !obj->isCall());

How about !IsCacheableNonGlobalScope(obj) instead of !obj->isCall()?

/be
Comment 59 David Mandelin [:dmandelin] 2011-03-01 11:52:37 PST
Comment on attachment 514593 [details] [diff] [review]
Give blocks and call objects unique shapes when they have parents that may be extended with new bindings.

Given that the bug requires |let|, I don't think we should take a nontrivial patch this close to release.
Comment 60 Jim Blandy :jimb 2011-03-02 02:13:51 PST
(In reply to comment #59)
> Given that the bug requires |let|, I don't think we should take a nontrivial
> patch this close to release.

Sorry, the bug summary is misleading; comment 56 shows pure ES*3* code that triggers the bug. Please re-add the approval2.0? flag if you agree.
Comment 61 David Mandelin [:dmandelin] 2011-03-02 09:50:32 PST
(In reply to comment #60)
> (In reply to comment #59)
> > Given that the bug requires |let|, I don't think we should take a nontrivial
> > patch this close to release.
> 
> Sorry, the bug summary is misleading; comment 56 shows pure ES*3* code that
> triggers the bug. Please re-add the approval2.0? flag if you agree.

OK. My other concern was the risk of regressions--the patch is not small, although looking at it again I see it is mostly tests and comments. If you think it's very safe, could you post a brief explanation/risk analysis and renominate?
Comment 62 Jim Blandy :jimb 2011-03-03 12:13:05 PST
Its only intended effect is to give some scope chain objects unique shapes, which does nothing but cause property cache misses. There aren't major data structure changes, just some new flags in ordinary places. The most interesting code (the analysis) is on a well-beaten path: it's called on every compilation.

I don't think this is a big risk crash-wise.

It's more of a risk performance-wise, because the truth is we don't know how many more scope objects are going to get unique shapes in the wild; it depends on the code. But it has no effect on the Dromaeo JavaScript performance tests.
Comment 63 Jim Blandy :jimb 2011-03-03 17:30:32 PST
I just noticed that the widened assertion is tripped by the test jorendorff added two weeks ago for unfixed bug 458271 --- so that bug goes from failing an assertEq (which jit_test.py can catch) to hitting a JS_ASSERT (which jit_test.py can't catch).

Although I think the right thing going forward is to have that assert widened, that's not something we want to put in the release. So I'll remove the approval2.0 request.
Comment 64 Jim Blandy :jimb 2011-03-15 12:18:20 PDT
Created attachment 519477 [details] [diff] [review]
Revert test and assertion changes from bug 633890. r=jorendorff

r=jorendorff via IRC.

At present, AssertValidPropertyCacheHit doesn't catch most property cache
failures. I would like to fix it, but doing so makes
jit-test/tests/basic/bug633890.js fail with a C++ assertion, not a JS shell
assertion, which jit-test/jit_test.py can't handle.

I think it's important to have AssertValidPropertyCacheHit doing its job;
it would have caught problems like bug 554955 earlier. Since bug633890.js
is a known failure (bug 458271), deleting it doesn't reduce coverage. 

This patch deletes that test, and reverts the assertion changes made in bug
633890, as suggested by jorendorff. A later patch in the series fixes
AssertValidPropertyCacheHit.
Comment 65 Jim Blandy :jimb 2011-03-15 12:20:08 PDT
http://hg.mozilla.org/tracemonkey/rev/67b102d581dd
Comment 66 Chris Leary [:cdleary] (not checking bugmail) 2011-03-31 14:29:51 PDT
http://hg.mozilla.org/mozilla-central/rev/67b102d581dd

Note You need to log in before you can comment on or make changes to this bug.