Closed Bug 593129 Opened 14 years ago Closed 12 years ago

Clean up jsscope.cpp methods to be object-independent

Categories

(Core :: JavaScript Engine, defect, P2)

defect

Tracking

()

RESOLVED FIXED

People

(Reporter: brendan, Assigned: brendan)

References

Details

This entails:

* Re-engineering JSObject::{alloc,free}Slot so the jsobj.cpp code grows and shrinks dslots only after successfully reshaping the object's shape-tree path or dictionary list.

* Similarly, relocating JSObject::{lastProp,flags} update logic as needed from jsscope.cpp to jsobj.cpp.

* Renaming methods, especially "putProperty", avoiding JSObject and "Property" names and tropes.

* Renaming jsscope.{cpp,h} to jsshape.{cpp,h}, although we are due for a bigger renaming fest.

Some of this might wait till later, but the first three bullets (first two for sure) should happen soon.

/be
Depends on: 592556
Assignee: general → brendan
I noticed this while reviewing another bug, but it makes sense to fix it here:

>    /*
>     * Private pointer to the last added property and methods to manipulate the
>     * list it links among properties in this scope. The {remove,insert} pair
>     * for DictionaryProperties assert that the scope is in dictionary mode and
>     * any reachable properties are flagged as dictionary properties.

This remains in jsobj.h, as a comment on the union for lastprop and map. It's not a very good comment.

I hope we can kill map, so there's no union, only lastProp, which is simply NULL for non-native objects. The comment can then say,

     /*
      * If isNative(), lastProp points to the most recently added of this
      * object's own properties, or an empty shape if this object has no
      * own properties.
      *
      * If !isNative(), lastProp is null.
      *
      * If inDictionaryMode(), then lastProp and all its siblings are
      * dictionary properties, forming a doubly-linked list. But this does
      * not ensure that this object owns all those properties; they may
      * be shared and frozen (for Call objects; see u.i.names in
      * JSFunction).
      */
Oh yeah, map and the union are marked for death!

/be
Minor TODO item:

         * and caches may return get deleted DictionaryShapes! See bug 595365.

comment just added to JSObject::removeProperty has an extraneous "get", and also does fails to advert to the changes in the same cset (for bug 595365) that reject property cache fill attempts when adding to dictionary-mode objects.

Both changes in the fix for bug 595365 are good, for sanity, defense in depth, and future-proofing. The comment just needs work.

/be
Shared dictionary shapes are marked frozen already (shared among functions via u.i.names, and their Call objects). We want a similar notion of frozen to apply to the PropertyTables of shapes in the shared shape forest.

/be
Blocks: 367425
Shared (frozen) dictionary shapes were a late addition to the deep patch queue for bug 558451, and the check to clone a dictionary list is done only in JSObject::getChildProperty. Not in JSObject::changeProperty or JSObject::putProperty. This is a theoretical or future-hostile thing, not an actual problem, since Call objects cannot be accessed in-language and subject to change/putProperty.

Separate issue: dictionary-mode objects sometimes need OWN_SHAPE set to ensure unique shape, e.g. when removing a non-last property. But changeProperty does not take advantage of the mutable, unshared status of unfrozen dictionary shapes and simply update the members that are changing _in situ_. Worse, if OWN_SHAPE is set then changeProperty's call to udpateShape will regenerate objShape unnecessarily in the case of the last property being the one that is changed.

Of course changeProperty always updates a dictionary-mode object's lastProp, removing the old shape and allocating a new one with the changes and adding it as the new lastProp, so right now any change, to last or non-last property, needlessly regenerates objShape if OWN_SHAPE, when the new (uniquely shaped) dictionary property at lastProp suffices.

/be
(In reply to comment #5)
> Shared (frozen) dictionary shapes were a late addition to the deep patch queue
> for bug 558451, and the check to clone a dictionary list is done only in
> JSObject::getChildProperty. Not in JSObject::changeProperty or
> JSObject::putProperty. This is a theoretical or future-hostile thing, not an
> actual problem, since Call objects cannot be accessed in-language and subject
> to change/putProperty.

Agreed. changeProperty and putProperty should assert that, though.

> Separate issue: dictionary-mode objects sometimes need OWN_SHAPE set to ensure
> unique shape, e.g. when removing a non-last property. But changeProperty does
> not take advantage of the mutable, unshared status of unfrozen dictionary
> shapes and simply update the members that are changing _in situ_. Worse, if
> OWN_SHAPE is set then changeProperty's call to udpateShape will regenerate
> objShape unnecessarily in the case of the last property being the one that is
> changed.
>
> Of course changeProperty always updates a dictionary-mode object's lastProp,
> removing the old shape and allocating a new one with the changes and adding it
> as the new lastProp, so right now any change, to last or non-last property,
> needlessly regenerates objShape if OWN_SHAPE, when the new (uniquely shaped)
> dictionary property at lastProp suffices.

It's usually necessary to update objShape here, right? Basic layout guarantee!

Do you just mean that we call js_GenerateShape more than needed, using up shape-ids?
(In reply to comment #6)
> Agreed. changeProperty and putProperty should assert that, though.

You bet, if they don't do something better (or go away: change looks worth its weight but put is perilously close to remove/add without the commented-upon slot preservation magic).

> > Separate issue: dictionary-mode objects sometimes need OWN_SHAPE set to ensure
> > unique shape, e.g. when removing a non-last property. But changeProperty does
> > not take advantage of the mutable, unshared status of unfrozen dictionary
> > shapes and simply update the members that are changing _in situ_. Worse, if
> > OWN_SHAPE is set then changeProperty's call to udpateShape will regenerate
> > objShape unnecessarily in the case of the last property being the one that is
> > changed.

Yes, I'm just noting excessive generation of shape ids.

> > Of course changeProperty always updates a dictionary-mode object's lastProp,
> > removing the old shape and allocating a new one with the changes and adding it
> > as the new lastProp, so right now any change, to last or non-last property,
> > needlessly regenerates objShape if OWN_SHAPE, when the new (uniquely shaped)
> > dictionary property at lastProp suffices.

Note that the "removing the old shape and allocating a new one" was done when I thought I might eliminate uint32 shape identifiers, but that's not going to happen, short run, and it is not obviously more winning to use ~8 words of main memory instead of a 32-bit int for hard cases. Losing shape ids also eliminates teleportation, which matters for deep-proto-chain code (gmail, other Google closure compiler code).

So I never went back and conserved the dictionary-mode, non-frozen shape being changed, 'sall.

> It's usually necessary to update objShape here, right? Basic layout guarantee!

Always -- funnels through updateShape -> setOwnShape(js_GenerateShape(...) or direct assignment to objShape. My point was that we do not need to generate given the uniquely-shaped dprop.

> Do you just mean that we call js_GenerateShape more than needed, using up
> shape-ids?

Exactly.

/be
Thanks for the stimulating comments -- keep 'em coming!

/be
Target Milestone: mozilla2.0b7 → mozilla2.0b8
Target Milestone: mozilla2.0b8 → ---
This looks obsolete by now. bhackett did the deed in bug 684505 or another bug or bugs. Please deny and reopen if I'm wrong. Thanks!

/be
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.