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

RESOLVED DUPLICATE of bug 558754

Status

()

defect
RESOLVED DUPLICATE of bug 558754
10 years ago
9 years ago

People

(Reporter: brendan, Assigned: gal)

Tracking

Trunk
mozilla2.0
Points:
---
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

()

Attachments

(3 attachments, 3 obsolete attachments)

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

/be
Assignee: general → jwalden+bmo
Posted patch patch (obsolete) — Splinter Review
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.
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 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
Posted patch patchSplinter Review
stealing with permission
Assignee: jwalden+bmo → gal
Attachment #431567 - Attachment is obsolete: true
Attachment #431934 - Flags: review?(brendan)
Status: NEW → ASSIGNED
Flags: in-testsuite?
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
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
Attachment #431965 - Attachment is obsolete: true
Attachment #431967 - Attachment is obsolete: true
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
Assignee: gal → brendan
Attachment #431934 - Flags: review?(brendan) → review-
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
Oops, already stole it!

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

/be
Assignee: brendan → gal
Ahem, don't keep dups on your list :-P.

/be
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: fastiterators
Target Milestone: mozilla1.9.3 → mozilla2.0
You need to log in before you can comment on or make changes to this bug.