|var obj = {}; Object.defineProperty(obj, 2, { configurable: true }); delete obj[2];| results in a dictionary-mode object

ASSIGNED
Assigned to

Status

()

Core
JavaScript Engine
--
minor
ASSIGNED
5 years ago
4 years ago

People

(Reporter: Waldo, Assigned: isk)

Tracking

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

5 years ago
Consider:

  var obj = {};
  Object.defineProperty(obj, 2, { configurable: true });
  delete obj[2];

The definition adds a non-dictionary-mode property and makes |obj| indexed.  Deleting the property should simply be rolling back the shape to lastProperty()->previous().  But this deletion happens to fail JSObject::canRemoveLastProperty():

    js::Shape *previous = lastProperty()->previous().get();
    return previous->getObjectParent() == lastProperty()->getObjectParent()
        && previous->getObjectMetadata() == lastProperty()->getObjectMetadata()
        && previous->getObjectFlags() == lastProperty()->getObjectFlags();

The first two checks pass, but the last one fails, because |previous| doesn't have the INDEXED flag, while |lastProperty()| obviously must.

We do want INDEXED propagating to descendant shapes, but it should be fine to roll back the shape tree and lose it.  I think that would call for a new kind of flag -- not shapeful but in BaseShape to save space, not object-ful in that it can be removed.  It's probably not worth the trouble of anyone going out of their way to fix this, but it might be worth doing as a learning experience about shape flags.
(Assignee)

Comment 1

4 years ago
This bug has been fixed?
I didn't fail JSObject::canRemoveLastProperty (I check it using gdb)
I ckeck return value too.
>js>var obj = {}
>js>Object.defineProperty(obj,2,{configurable:true})
>js>delete obj[2]
>true
Flags: needinfo?(jwalden+bmo)
(Reporter)

Comment 2

4 years ago
This bug isn't visible as a JS semantic issue -- it's only visible in property accesses in certain cases getting slower on |obj|.  (Assuming, of course, that dictionary-mode objects don't have bugs that non-dictionary-mode objects don't have, but that's not really this bug's fault, then.)

Looking at vm/Shape.h, INDEXED is still part of OBJECT_FLAG_MASK, used by getObjectFlags(), so I'm pretty sure the issue is still present.  And indeed, this shell transcript shows the issue still exists:

js> var obj = {}
js> Object.defineProperty(obj,2,{configurable:true})
({})
js> delete obj[2]
true
js> dumpObject(obj)
object 0x7f6f4806c540
class 0x1aa2ae8 Object
flags: indexed inDictionaryMode hasShapeTable
proto <Object at 0x7f6f48040020>
parent <global object at 0x7f6f4803e060>
properties:

js> var obj = {}
js> dumpObject(obj)
object 0x7f6f4806c5c0
class 0x1aa2ae8 Object
flags:
proto <Object at 0x7f6f48040020>
parent <global object at 0x7f6f4803e060>
properties:


That first dumpObject shouldn't include inDictionaryMode or indexed, if it's just rolling back the shape tree.
Flags: needinfo?(jwalden+bmo)
(Assignee)

Comment 3

4 years ago
Created attachment 8338378 [details] [diff] [review]
bug880910.patch

Thank you for commenting.
I could understand where is wrong thanks to you.

>We do want INDEXED propagating to descendant shapes, but it should be fine to roll back the shape
>tree and lose it.  I think that would call for a new kind of flag -- not shapeful but in BaseShape to 
> save space, not object-ful in that it can be removed.  It's probably not worth the trouble of 
> anyone going out of their way to fix this, but it might be worth doing as a learning experience about 
> shape flags.

It is good idea but I suggest another approach in this WIP-patch.
I think "previous->getObjectFlags() == 0" means undefined because flags cannot be unset(described in vm/Shape.h:558).
So If previous->getObjectFlags() is 0, it can through "previous->getObjectFlags() == lastProperty()->getObjectFlags()"

This WIP-patch pass jit-tests.
Assignee: general → iseki.m.aa
Attachment #8338378 - Flags: feedback?(jwalden+bmo)
(Reporter)

Comment 4

4 years ago
Comment on attachment 8338378 [details] [diff] [review]
bug880910.patch

Review of attachment 8338378 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jsobjinlines.h
@@ +138,5 @@
>      js::Shape *previous = lastProperty()->previous().get();
>      return previous->getObjectParent() == lastProperty()->getObjectParent()
>          && previous->getObjectMetadata() == lastProperty()->getObjectMetadata()
> +        && (   previous->getObjectFlags() == lastProperty()->getObjectFlags()
> +            || previous->getObjectFlags() == 0 );

The existing code/style is wrong, we'd format it this way normally (and probably fix the existing code at the same time), if we took this:

    return previous->getObjectParent() == lastProperty()->getObjectParent() &&
           previous->getObjectMetadata() == lastProperty()->getObjectMetadata() &&
           (previous->getObjectFlags() == lastProperty()->getObjectFlags() ||
            previous->getObjectFlags() == 0);

I'm not sure on first read if this is, or is not, a fully correct change.  Still thinking about this -- although, the brutal truth is I probably shouldn't think *too* much longer on this, as deletion is rare enough that optimizations to it don't have enough payoff to make it worth the effort.  Tomorrow!
(Reporter)

Comment 5

4 years ago
Comment on attachment 8338378 [details] [diff] [review]
bug880910.patch

Review of attachment 8338378 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jsobjinlines.h
@@ +138,5 @@
>      js::Shape *previous = lastProperty()->previous().get();
>      return previous->getObjectParent() == lastProperty()->getObjectParent()
>          && previous->getObjectMetadata() == lastProperty()->getObjectMetadata()
> +        && (   previous->getObjectFlags() == lastProperty()->getObjectFlags()
> +            || previous->getObjectFlags() == 0 );

No, this is wrong.  Testcase:

  var obj = {};
  obj.a = 5;
  obj.b = 7;
  obj.watch("c", function(){});
  delete obj.b;

This last line hits this code, and it's the case that |previous->getObjectFlags() != lastProperty()->getObjectFlags()| -- which makes this fail with the current code -- but |previous->getObjectFlags() == 0| -- which makes this pass with the new code.  Rolling back the property, however, would mean that we'd lose the WATCHED flag.  Which is not the end of the world, it's WATCHED, but I don't think there's anything special here about it being WATCHED specifically, and not some other more-important flag.

So: this isn't the way to improve things here.
Attachment #8338378 - Flags: feedback?(jwalden+bmo) → feedback-
(Reporter)

Comment 6

4 years ago
Yeah, if instead of the watch call you had Object.preventExtensions(obj), that'd trigger in the exact same way and lose the non-extensible bit.
(Assignee)

Comment 7

4 years ago
Thanks for your feedback.
Sorry for sending incomplete patch.

I didn't understand well.
I have a few question.

1. What is the mean of ObjectMetadata?
2. Why If shape is not the last property added, or the last property cannot be removed, switch to dictionary mode? (where shape is in JSObject::removeProperty)
3. What is expected result?

Current code's result is following.

js> var obj = {}
js> Object.defineProperty(obj,2,{configurable:true})
({})
js> delete obj[2]
true
js> dumpObject(obj)
object 0x7f6f4806c540
class 0x1aa2ae8 Object
flags: indexed inDictionaryMode hasShapeTable
proto <Object at 0x7f6f48040020>
parent <global object at 0x7f6f4803e060>
properties:

expected result is following right?

js> var obj = {}
js> Object.defineProperty(obj,2,{configurable:true})
({})
js> delete obj[2]
true
js> dumpObject(obj)
object 0x7f6f4806c540
class 0x1aa2ae8 Object
flags: indexed
proto <Object at 0x7f6f48040020>
parent <global object at 0x7f6f4803e060>
properties:
(Assignee)

Updated

4 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 8

4 years ago
ping
Flags: needinfo?(jwalden+bmo)
(Reporter)

Comment 9

4 years ago
(In reply to iseki.m.aa from comment #7)
> 1. What is the mean of ObjectMetadata?

It's extra data you can associate with an object.  I think we use it to track allocation location, or something like that, in debugging tools.  Exactly what it is, probably isn't too important.

> 2. Why If shape is not the last property added, or the last property cannot
> be removed, switch to dictionary mode? (where shape is in
> JSObject::removeProperty)

Objects store their properties in a sequence.  This sequence is the ordering that's exposed by things like for-in and such.  This sequence is itself part of a tree of shapes -- shapes in this tree can be shared by multiple objects that look similar enough.  Deleting a property has to remove a property from the object's shape sequence.  But because these shapes are shared, you can't just remove it -- else you'd affect other objects.  So when the shape can be rolled back, to delete a property, it is.  But if it's in the middle, or for some reason the previous shape isn't suitable for the current object, you have to create a whole new sequence of shapes.  And because deletes are rare, it's easiest to just give the object its own sequence of shapes not shared with anything.

Or did I misunderstand your question to be asking something more complicated than the basics of how our shape mechanism works?

> 3. What is expected result?
> 
> Current code's result is following.
> 
> js> var obj = {}
> js> Object.defineProperty(obj,2,{configurable:true})
> ({})
> js> delete obj[2]
> true
> js> dumpObject(obj)
> object 0x7f6f4806c540
> class 0x1aa2ae8 Object
> flags: indexed inDictionaryMode hasShapeTable
> proto <Object at 0x7f6f48040020>
> parent <global object at 0x7f6f4803e060>
> properties:
> 
> expected result is following right?
> 
> js> var obj = {}
> js> Object.defineProperty(obj,2,{configurable:true})
> ({})
> js> delete obj[2]
> true
> js> dumpObject(obj)
> object 0x7f6f4806c540
> class 0x1aa2ae8 Object
> flags: indexed
> proto <Object at 0x7f6f48040020>
> parent <global object at 0x7f6f4803e060>
> properties:

Yeah, that's the expected result.  Possibly with hasShapeTable, that flag's a lazily-computed thing, so if you don't have it at first but pick it up later that's not a problem on its own.
Flags: needinfo?(jwalden+bmo)
(Assignee)

Comment 10

4 years ago
Thank you for your answering.

I don't know how to fix this bug.

>We do want INDEXED propagating to descendant shapes, but it should be fine to roll back the shape tree and lose it. 
>I think that would call for a new kind of flag -- not shapeful but in BaseShape to save space,
>not object-ful in that it can be removed. 
>It's probably not worth the trouble of anyone going out of their way to fix this, 
>but it might be worth doing as a learning experience about shape flags.

A new kind of flag is only for INDEXED?
(Reporter)

Comment 11

4 years ago
(In reply to iseki.m.aa from comment #10)
> I don't know how to fix this bug.

Yeah, I don't think there's a super-simple answer here.

> A new kind of flag is only for INDEXED?

A new *mask* of flags, that is.  Currently object flags adhere to the object.  Removing a property, adding a property, etc. don't change them.  But INDEXED is a special case, because it's a property of the object, but it's one that can be imputed from the sequence of properties.  And when the sequence is in order, and the previous property *doesn't* have INDEXED but the current one does, it's safe to remove the INDEXED object flag, where it's not safe to remove any other object flag.  This makes INDEXED different from everything else, and it requires a different way to deal with updates to it.

None of this is especially easy/obvious/clear, not with our current level of code documentation.  Hence why I didn't flag this as an especially good thing to work on, right off the bat.  But it's an issue that can be fixed, and probably eventually should be, so I filed it.
(Assignee)

Comment 12

4 years ago
Created attachment 8348563 [details] [diff] [review]
Bug880910.patch

I add a condition to propagate INDEXED flag only.
Attachment #8338378 - Attachment is obsolete: true
Attachment #8348563 - Flags: review?(jwalden+bmo)
(Reporter)

Comment 13

4 years ago
Comment on attachment 8348563 [details] [diff] [review]
Bug880910.patch

Review of attachment 8348563 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jsobjinlines.h
@@ +138,5 @@
>      js::Shape *previous = lastProperty()->previous().get();
> +    return previous->getObjectParent() == lastProperty()->getObjectParent() &&
> +           previous->getObjectMetadata() == lastProperty()->getObjectMetadata() &&
> +           (previous->getObjectFlags() == lastProperty()->getObjectFlags() ||
> +            lastProperty()->getObjectFlags() == js::BaseShape::Flag::INDEXED);

I'm pretty sure this is *correct*, as far as it goes.  But if we're going to go to the effort to fix this, I think we should *fully* fix it -- for all cases where the INDEXED bit is the only difference between current object flags and previous ones.

For example, with your patch I get this behavior (in a build with JSObject::dump adjusted to print out a few more of the object flags):

[jwalden@find-waldo-now src]$ dbg/js
js> var obj = {}; obj.a = 5;       
5
js> obj[(function() { return 'a' })()] = 7;  obj[55555555] = 7
7
js> dumpObject(obj)
object 0x7f8e80f4cc00
class 0x1aaaeb0 Object
flags: indexed had_elements_access
proto <Object at 0x7f8e80f3e020>
parent <global object at 0x7f8e80f3c060>
properties:
    ((JSShape *) 0x7f8e80f502b8) enumerate JSString* (0x7f8e81100c40) = jschar * (0x7f8e81100c50) = "a"
: slot 0 = 7
    ((JSShape *) 0x7f8e80f502e0) enumerate 55555555: slot 1 = 7

js> delete obj[55555555]
true
js> dumpObject(obj)
object 0x7f8e80f4cc00
class 0x1aaaeb0 Object
flags: indexed had_elements_access inDictionaryMode hasShapeTable
proto <Object at 0x7f8e80f3e020>
parent <global object at 0x7f8e80f3c060>
properties:
    ((JSShape *) 0x7f8e80f50380) enumerate JSString* (0x7f8e81100c40) = jschar * (0x7f8e81100c50) = "a"
: slot 0 = 7

There's really not a good reason for that example to fall back into inDictionaryMode again.

Something like comparing

  (lastProperty()->getObjectFlags() & ~js::BaseShape::Flag::INDEXED)

and

  (previous->getObjectFlags() & ~js::BaseShape::Flag::INDEXED)

for equality is a full, complete fix.  But I'm not sure what I think about doing totally one-off INDEXED masking-out here.  Probably another flags mask for this particular purpose is desirable, akin to OBJECT_FLAGS_MASK, but I don't know how I'd name it.

Also, given recent bugmail scrollby, it sounds like TI has closer interactions with this method than I may have understood it to have.  If so, a TI fix may be needed in addition to this.  Try poking bhackett to see if there is actually any sort of issue along those lines, before posting any new patches.
Attachment #8348563 - Flags: review?(jwalden+bmo)
You need to log in before you can comment on or make changes to this bug.