548903 - for-in loop should snapshot direct and prototype properties with shadowing, up front

Status: RESOLVED DUPLICATE of bug 558754

(Core :: JavaScript Engine, defect)

Description

[Brendan Eich [:brendan]]

This avoids pathological duplicate-id results, matches other browsers, and can use a faster shadowing check to boot.

/be

Comment 1

[Andreas Gal :gal]

Attachment: patch

Brendan and I came up with this while discussing this problem. Have to show this to Waldo and see what he thinks. This speeds up fasta a bit. Now 22% is spent in making the iterator object.

Comment 2

[Jeff Walden [:Waldo]]

It occurred to me, skimming this, that if you cached a shape in the iterator, you could compare the pre-iteration shape to the mid-iteration shape and verify that properties haven't been deleted that way.  Any deletion in the object would kill you, but eh, that seems fine enough, it's not the common case.

Comment 3

[Brendan Eich [:brendan]]

Comment on attachment 431567 [details] [diff] [review]
patch

>     STOBJ_SET_SLOT(iterobj, JSSLOT_ITER_FLAGS, INT_TO_JSVAL(flags));

Nit: blank line here.

>+    /*
>+     * For native objects sample the property removal counter here. We will use it to
..12345678901234567890123456789012345678901234567890123456789012345678901234567890

>+     * detect during iteration whether we have to re-scan the scope for properties
>+     * having been potentially deleted during the iteration. For non-native objects,
>+     * we always perform that check. We also disable the fast path if the class of
>+     * the object has an outerObject hook, since additional properties might have
>+     * been injected by it.

Uber-nit: ragged right margin is not consistent with any wrapmargin setting (wm in vim). For comments let's stick to readable old virtual-80 (wm=79).

>+     */
>+    JSClass *clasp;
>+    jsval sample = (!obj->isNative() ||
>+                    (((clasp = obj->getClass())->flags & JSCLASS_IS_EXTENDED) &&
>+                     ((JSExtendedClass *) clasp)->outerObject))
>+        ? JSVAL_VOID
>+        : INT_TO_JSVAL(cx->runtime->propertyRemovals & 0x7fffffff);

Super-duper-nit: indent the ? and : to underhang the leftmost ( of the condition.

>+        jsval sample = STOBJ_GET_SLOT(iterobj, JSSLOT_ITER_SAMPLE);
>+        if (sample == JSVAL_VOID || sample != INT_TO_JSVAL(cx->runtime->propertyRemovals& 0x7fffffff)) {

You don't need the JSVAL_VOID test, since it must be a distinct jsval from any INT jsval.

You forgot to update jsscope.cpp to bump propertyRemovals even when there's no slot for the property being removed.

This is a minimal solution that wins perf. I say we get a revised patch in, ASAP.

Waldo, checking shape is certainly doable but more work, and delete during for-in just never happens. For ECMA-262 correctness we have to care, but we shouldn't spend another byte of code or minute of bug comment editing worrying about it.

/be

Comment 4

[Andreas Gal :gal]

Attachment: patch

stealing with permission

Comment 5

[Brendan Eich [:brendan]]

Attachment: Kent Hansen's original HTML testcase

Comment 6

[Brendan Eich [:brendan]]

Attachment: js shell test hacked from Kent's test

Comment 7

[Brendan Eich [:brendan]]

Results without patch:

Enumeration order: 
a b
Case I: 
a b
Case II: 
a a
Case III: 
a
Case IV: 
a c

Again the cases are:

Case I: Add a property to O's prototype.
Case II: Delete a property on O and add a property of the same name on the prototype.
Case III: Add a property to O so that the prototype's property of the same name becomes "shadowed". (Credit goes to Oliver Hunt for this one)
Case IV: Change O's [[Prototype]] property.

With the patch, the results are:

Enumeration order: 
a b
Case I: 
a b
Case II: 
a a
Case III: 
a b
Case IV: 
a c

Safari 4.0.4 from Kent's es-discuss post gives:

Enumeration order: a b
Case I: a
Case II: a
Case III: a b
Case IV: a

If we want to snapshot the entire prototype chain, then we should match this. The patch so far fixes our case III output to be a b, but it does not fix the case I or (horrible) case II output.

/be

Comment 8

[Brendan Eich [:brendan]]

So the patch is a fine speedup and a partial fix to this bug. Spin it out to a new bug, or beef it up to fix the full bug reported here?

/be

Comment 9

[Brendan Eich [:brendan]]

Attachment: better js shell testcase -- emulates output line structure of HTML test

Comment 10

[Brendan Eich [:brendan]]

Attachment: even better shell testcase

Comment 11

[Brendan Eich [:brendan]]

Also, the patch is broken:

$ ./Darwin_DBG.OBJ/js
js> var p = {x:1}
js> var o = Object.create(p)
js> o.x = 2
2
js> for (i in o) print(i)
x
x

That lookupProperty is not just for deleted property suppression -- it performs the shadowing property suppression.

Tagging into the ring, I'll take the patch further.

/be

Comment 12

[Brendan Eich [:brendan]]

Comment on attachment 431934 [details] [diff] [review]
patch

Broken as noted -- I have a derived patch in my mq, probably I should steal this bug.

/be

Comment 13

[Brendan Eich [:brendan]]

Oops, already stole it!

/be

Comment 14

[Brendan Eich [:brendan]]

Tagging out of ring for stack-shrunk-wrapped vars. Andreas is on this.

/be

Comment 15

[Brendan Eich [:brendan]]

Ahem, don't keep dups on your list :-P.

/be