Closed Bug 751377 Opened 12 years ago Closed 1 year ago

Implement properties in the new object representation

Categories

(Core :: JavaScript Engine, task)

task

Tracking

()

RESOLVED FIXED

People

(Reporter: Waldo, Unassigned)

References

Details

(Whiteboard: [js:t][Leave open after merge])

Attachments

(8 files, 1 obsolete file)

We need properties implemented in order to be able to test pretty much anything, because creating anything requires a global and all that jazz, and that all requires named properties.

Because we want to be able to easily flip a switch between old representation and new, there are two options: rewrite the existing code, adapt it, or copy it.  There's an awful lot of knowledge in the current code, and rewriting seems a bit crazy.  Adapting is tempting, but the requisite #ifdefs would present issues.  Particularly, it would make it hard to work on the new representation incrementally, leaning on the compiler for compilability along the way, the way I've been able to do thus far for ElementsHeader (new) versus ObjectElements (old).  So that seems out.  This leaves copying, which will allow incrementally building up the new code while preserving the old, and eventually allowing a compile-time switch between the two.

I'm keeping the concepts of the current implementation in the new one, more or less, but I'm making various small improvements along the way.  The current naming is misnomer-ish (shape versus property in particular), so I'll be improving that as I go (plus it's necessary to have side-by-side implementations as I go).  I'm introducing derived classes to separate owned-property implementation details from shared-property details.  I'm also adding a class that encapsulates a property pointer that's really used as a list of properties; it'll also contain various methods currently on Shape that are more about list functionality than about single-property functionality.  A bunch of methods currently on JSObject will move into this new class at the same time, I expect.

All this functionality's pretty well intertwined, so break points between patches will probably be a bit artificial, unfortunately.  This bug's going to be around for a fair number of inbound patches, for sure.
New stuff will be named based on "Property" rather than "Shape".  The current "PropertyTable" kind of gets in the way of having sibling implementations until a final switch, however.  So let's rename it to ShapeTable so that we can introduce PropertyTable for the same concept in the new implementation.

This is a search-and-replace for PropertyTable first, then for "property table" case-insensitively in comments second -- nothing much interesting.
Attachment #620477 - Flags: review?(bhackett1024)
Attachment #620477 - Flags: review?(bhackett1024) → review+
Sorry, backed out on inbound because of build failure:
https://hg.mozilla.org/integration/mozilla-inbound/rev/26738df8a4e0
https://hg.mozilla.org/integration/mozilla-inbound/rev/6ead5d74cb03 for the renaming patch (it happened to be atop another patch that was faulty and didn't have an immediately obvious fix, and it seemed unwise to attempt a surgical backout).
Sorry this is a bit big.  The existing structures are all pretty intertwined, and I wasn't thinking and let the set of structures ported in a first patch get overly big.

The implementation of all this (some of which is here, some of which isn't, some of which I'm sure I'll have to update for friend-API shadow uses at some point) will basically parallel the existing one.  I'm trying to organize it a bit more clearly (I hope), and I'm adding documentation to things to hopefully tie it all together a bit better as I go.  But right now it doesn't do anything on its own, and it won't, likely, until many more incremental porting patches happen.  (And of course the actual switchover to use any of this is a bit down the road regardless.)  Small steps, small steps.
Attachment #622401 - Flags: review?(bhackett1024)
Comment on attachment 622401 [details] [diff] [review]
Starts on new property data structures

bhackett thought this was too world-changing and not enough incremental-ing.  I can see where he's coming from, although I think I disagree.  But enough of that, hopefully with some work I can make things copacetic working on the existing code and not changing things, maybe.  Back soon with more, with a different tack.
Attachment #622401 - Flags: review?(bhackett1024)
Sorry for being so sluggish here, I was trying to think about how best to rejigger things to avoid the ton of code duplication.  It seems like if the problem is that Shape contains both property stuff (which crazy arrays with indexed setters etc. will need to think about) and property hierarchy stuff, you could split the existing Shape into a Property class and a Shape class which inherits from the Property class and doesn't duplicate functionality.  Less world changing would be to just use Shapes directly in the crazy array case but just leave the hierarchy fields empty, this won't affect perf/memory usage (crazy arrays should be super rare) but it's more about the cleanliness of the result.
Whiteboard: [Leave open after merge] → [js:t][Leave open after merge]
Depends on: 758913
I've removed a few of the resolve flags in the last couple weeks, but it seems implausible that I'll be able to remove all of them in the time frame the property work wants, especially since at least some embedders use them.  So let's just add them to the APIs and propagate them through.  :-\  This adds them to all the element methods; the property methods to be implemented as siblings to these will need the same resolve flags stuff too, of course.
Attachment #630724 - Flags: review?(bhackett1024)
We store jsid in Shapes right now, but jsid allows integers, and in the long run we shouldn't allow that, in the interest of avoiding errors.  This needs to appear now so that I can implement GetOwnProperty/GetProperty/SetProperty and so on methods as siblings to the element versions, without too-narrowly using PropertyName* as the key type.

There is some overlap/interdependency of the representations of jsid, SpecialId, and PropertyKey.  Until we can remove jsid entirely, I don't see a great way to make that overlap particularly cleaner.  (Note that PropertyKey is an internal detail of shapes and such, and I don't see a reason why we would need or want to expose it to JSAPI users, even as a friend-y thing.)
Attachment #630769 - Flags: review?(bhackett1024)
This shows the basic idea of how PropertyKey would be used in practice, i.e. you really wouldn't if you were outside the shape tree code, and you'd use a PropertyName* or SpecialId-based version.
Attachment #630773 - Flags: review?(bhackett1024)
The wind's blowing this way, gotta get with it.
Attachment #630773 - Attachment is obsolete: true
Attachment #630773 - Flags: review?(bhackett1024)
Attachment #632736 - Flags: review?(bhackett1024)
Comment on attachment 630724 [details] [diff] [review]
Add resolve flags to all the element APIs

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

::: js/src/vm/ObjectImpl.cpp
@@ +291,5 @@
>  }
>  
>  bool
> +DenseElementsHeader::getOwnElement(JSContext *cx, ObjectImpl *obj, uint32_t index, unsigned flags,
> +                                   PropDesc *desc)

Can you use 'resolveFlags' instead of 'flags' for this argument and the others in this patch?
Attachment #630724 - Flags: review?(bhackett1024) → review+
Comment on attachment 630769 [details] [diff] [review]
Introduce a PropertyKey type for the name of properties represented using shapes

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

::: js/src/vm/ObjectImpl.h
@@ +47,5 @@
> + * 32-bit integers) or SpecialIds (object values for E4X and a couple other
> + * things, see jsid for details); the union of these types, used in individual
> + * shapes, is PropertyKey.
> + */
> +class PropertyKey

Would PropertyId work better as the name here?  It makes it clearer that this is related to jsid, and not the key in the hashtable (what I usually think of the Key suffix as meaning).

@@ +52,5 @@
> +{
> +    uintptr_t bits;
> +
> +    static const uintptr_t NameBits = 0x0;
> +    static const uintptr_t TypeMask = 0x7;

It would be nice if these masks were not necessary and the methods below could be implemented using existing jsid tests and asId().
Attachment #630769 - Flags: review?(bhackett1024) → review+
Attachment #632736 - Flags: review?(bhackett1024) → review+
Comment on attachment 632737 [details] [diff] [review]
Implement [[GetOwnProperty]] for non-elements, atop the handle patch

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

::: js/src/vm/ObjectImpl.cpp
@@ +509,5 @@
> +        Class *clasp = obj->getClass();
> +        JSResolveOp resolve = clasp->resolve;
> +        if (resolve != JS_ResolveStub) {
> +            Rooted<jsid> id(cx, key.reference().asId());
> +            Rooted<JSObject*> robj(cx, static_cast<JSObject*>(obj.value()));

These should use RootedId and RootedObject.  It might be worth adding typedefs for roots/handles on PropertyKey and ObjectImpl too.
Attachment #632737 - Flags: review?(bhackett1024) → review+
Changed to resolveFlags, although it was a royal pain propagating that rename through my mq.  :-)

I changed to PropertyId, and I refactored the internals so right now PropertyId just wraps a jsid.

Current newsgroup consensus is for Rooted<...>, so I kept that as-is.

https://hg.mozilla.org/integration/mozilla-inbound/rev/fab79c2c9b9f
https://hg.mozilla.org/integration/mozilla-inbound/rev/df6b7367f1a4
https://hg.mozilla.org/integration/mozilla-inbound/rev/a16183d07a46
https://hg.mozilla.org/integration/mozilla-inbound/rev/53ba44b136f0
> Changed to resolveFlags, although it was a royal pain propagating that
> rename through my mq.  :-)

FWIW, I have found changes like that are sometimes easier to do by directly modifying the patch files in .hg/patches/.
This continues introduction of meta-object methods with one for [[GetProperty]].  I have a second patch atop this which starts to convert users over to this new signature; of course, the *implementation* of the signature's still #if-controlled, this just starts people using the new thing.
Attachment #656671 - Flags: review?(bhackett1024)
Attachment #656671 - Flags: review?(bhackett1024) → review+
Comment on attachment 656672 [details] [diff] [review]
Start getting users on the new property-getting methods

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

::: js/src/builtin/RegExp.cpp
@@ +243,4 @@
>           * 'toSource' is a permanent read-only property, so this is equivalent
>           * to executing RegExpObject::getSource on the unwrapped object.
>           */
> +        Rooted<Value> v(cx);

Can we please please please not go back and forth on whether these should be RootedValue or Rooted<Value>, same for all the other types?  It's annoying enough these aren't uniform, and just plain absurd to keep changing them from one to the other.

Existing usage in the codebase and conciseness both vote for RootedValue over Rooted<Value>, and I'd prefer if we were uniformly using the former.  If you disagree we can (all) talk about it next week, but there clearly needs to be a style guide.

::: js/src/jsobjinlines.h
@@ +1682,5 @@
>  
> +/* XXX move to ObjectImpl.h when the new representation is enabled */
> +inline bool
> +js::GetProperty(JSContext *cx, Handle<ObjectImpl*> obj, Handle<ObjectImpl*> receiver,
> +                Handle<PropertyName*> name, unsigned resolveFlags, MutableHandle<Value> vp)

I'd really prefer this method not exist, there are already way too many methods for getting properties off of objects and introducing new wrappers will just make things worse.

JSObject::getProperty and friends are themselves essentially wrappers, and it seems like you could just move the #if USE_NEW_OBJECT_REPRESENTATION logic there and convert all callers to use the new representation in one go.

::: js/src/vm/ObjectImpl.cpp
@@ +702,4 @@
>              args.setThis(ObjectValue(*current));
>  
>              bool ok = Invoke(cx, args);
> +            vp = args.rval();

vp.set(args.rval())

Unfortunately this will compile and do the wrong thing.  I've tried to fix this in gc/Root.h but the compiler stubbornly refuses to disallow this.  Maybe someone more versed in C++ could take a look.
Attachment #656672 - Flags: review?(bhackett1024)
https://hg.mozilla.org/integration/mozilla-inbound/rev/06eff7ce5bd7 for the first patch for now, looking into the comments on the second one.
Comment on attachment 656671 [details] [diff] [review]
Implement GetProperty/Element methods for the new representation

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

::: js/src/vm/ObjectImpl.cpp
@@ +592,5 @@
> +
> +    Rooted<ObjectImpl*> current(cx, obj);
> +
> +    do {
> +        MOZ_ASSERT(obj);

Did you mean to assert current here?
Depends on: 824217
Jeff, are you (or anybody else) still working on this?
Flags: needinfo?(jwalden+bmo)
I've been somewhat pulled onto other things these days, so I haven't had time to work on this much.  :-\  Still have in mind some of the prerequisite changes, and have been intermittently poking at them, but direct work on exactly this problem hasn't happened recently.
Flags: needinfo?(jwalden+bmo)
Depends on: 987007
Type: defect → task

The bug assignee is inactive on Bugzilla, so the assignee is being reset.

Assignee: jwalden → nobody
Status: ASSIGNED → NEW
Severity: normal → S3

This code has all been rewritten several times. Everything in this bug that needed doing was done elsewhere years ago.

Status: NEW → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.