739380 - Introduce a new class for representing elements

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jeff Walden [:Waldo]]

Attachment: Introduce an ElementsHeader class and subclasses

For ease of switching between the current object representation and the one being implemented for bug 586842, I'm going to implement the new representation piecemeal next to the current one.  Both bits of code will exist at the same time, but only one will actually be used (compile-time switch, eventually, but initially the new bits of code will be defined but just not called).

This patch introduces part of a skeleton of an elements class.  It's mostly not implemented, not used, will assert promptly if accidentally used, and won't work until a whole lot more patches land.  But it gets intermediate work in front of people sooner, it makes collaboration easier, and hopefully it'll make reviewing all the changes simpler, because it'll be splitting them up rather than presenting one mega-review.

Comment 1

[Jeff Walden [:Waldo]]

Attachment: Start adding element-definition methods

This starts adding js_DefineElement-style hook implementations to the element subclasses.  A notable exception is for SparseElements.  I'm still not certain whether it makes sense to reuse the existing Shape stuff to represent sparse elements, or whether I should write something new.  The shape stuff is pretty intricately tied to its current status as part and parcel of the object, without meaning outside it.  It's not clear to me whether it's easier to make that split to reuse code, or to implement a whole new thing better optimized for elements.  I think eventually we want the latter -- but we also want this done sooner rather than later, so delaying might be making perfect the enemy of good, and we should just do the expedient thing.  I'll decide what's the best move when I get further along.

It would be nice to be able to proof-of-concept some of the easier custom-elements stuff once implemented, without having to have it all implemented, i.e. get just enough stuff working to test that Uint8Array works properly, say.  Because of how bootstrapping works, that might not be possible right now (because we need a parent change, a standard prototype, that has to have Object.prototype working, etc.).  But maybe there's a way around that.  We'll see.

Comment 2

[David Mandelin [:dmandelin]]

(In reply to Jeff Walden (:Waldo) (remove +bmo to email) from comment #0)
> But it gets intermediate
> work in front of people sooner, it makes collaboration easier, and hopefully
> it'll make reviewing all the changes simpler, because it'll be splitting
> them up rather than presenting one mega-review.

This is a good thing. (Things?)

Comment 3

[Jeff Walden [:Waldo]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c7ce0c86d5e8
https://hg.mozilla.org/integration/mozilla-inbound/rev/e86e048872d6

More coming here, don't close.

Comment 4

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

https://hg.mozilla.org/mozilla-central/rev/c7ce0c86d5e8
https://hg.mozilla.org/mozilla-central/rev/e86e048872d6

Comment 5

[Jeff Walden [:Waldo]]

MSVC10 seems to think Uint8ClampedElementsHeader::defineElement needs to exist and be linked, so it doesn't build.  I removed the declaration of the method to fix that bustage:

https://hg.mozilla.org/integration/mozilla-inbound/rev/40406a497a3f
https://hg.mozilla.org/mozilla-central/rev/3801f97de347

Comment 6

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

https://hg.mozilla.org/mozilla-central/rev/40406a497a3f

Comment 7

[Jeff Walden [:Waldo]]

Attachment: Convert defineElement code to use the ECMAScript-style interface, not SpiderMonkey's current interface

Per the last comment in bug 586842, I'm switching the internal representation to match ECMAScript, moving away from js_GetProperty and js_DefineProperty APIs that have lots of extraneous stuff and don't have property descriptors and such.  This is a first step toward that, moving PropDesc into ObjectImpl and starting to define the relevant APIs on the element subclasses.

PropDesc has some of the things that are necessary for this -- specifically that fields can be omitted and such -- but it doesn't have everything (like a way to return a PropDesc representing |undefined|, the lack of a property).  Also its accessors lack any checks for correctness of accesses -- like asserting |hasWritable| in |writable()|, say.  One step at a time here...

This patch is atop the one in bug 743878, although I don't think there's much interaction between the two except in terms of line offsets.

Comment 8

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/92e87abb646b

Comment 9

[Jeff Walden [:Waldo]]

Attachment: Implement much of the [[GetP]] interface for the various element kinds

See http://wiki.ecmascript.org/doku.php?id=harmony:proto_climbing_refactoring for details of how [[GetP]] is currently proposed.

I haven't added /* Step 1. */-style comments yet, because the steps aren't necessarily set in stone, and a wiki page doesn't inspire particular confidence of permanency.  :-)  But I do want to add those comments when it's possible to do so.

Comment 10

[Jeff Walden [:Waldo]]

Attachment: Implement much of [[SetP]] for elements

The setting algorithm is a bit more complicated than the others, because of how it invokes multiple other hooks in its base algorithm, but I think it'll all roughly fall out the same way in implementation, I think.

Comment 11

[Jeff Walden [:Waldo]]

Attachment: Implement [[HasProperty]] for elements

This particular operation in the proto-walking stuff only calls [[GetOwnProperty]], which makes it almost a derived trap of sorts.  Thus there's no need for hasElement() methods or similar on all the elements headers.

Comment 12

[Brian Hackett [Laid off!]]

Comment on attachment 616760 [details] [diff] [review]
Implement much of the [[GetP]] interface for the various element kinds

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

::: js/src/vm/ObjectImpl.cpp
@@ +341,5 @@
> +        v = NumberValue(getElement(index));
> +    } else {
> +        /* Everything else fits in Int32Value. */
> +        v = Int32Value(getElement(index));
> +    }

It would be good if this could just use NumberValue() everywhere instead of case splitting.  Unnecessary double<->int conversions in some cases but oh well.

@@ +485,5 @@
> +               Value *vp)
> +{
> +    NEW_OBJECT_REPRESENTATION_ONLY();
> +
> +    JS_CHECK_RECURSION(cx, return false);

Why is this necessary?  Other object property-related methods don't have recursion checks.  If this is because of the Invoke(), RunScript under InvokeKernel will do its own recursion check.

@@ +556,5 @@
> +            *vp = desc.value();
> +            return true;
> +        }
> +
> +        /* If it's an accessor property, call its [[Set]] with the receiver. */

Should be [[Get]]

@@ +570,5 @@
> +                return false;
> +
> +            /* Push fval, thisv, and the args. */
> +            args.calleev() = get;
> +            args.thisv() = ObjectValue(*static_cast<JSObject *>(receiver)); // XXX

Can you add a constructor for ObjectValue that takes an ObjectImpl?  These static casts are confusing.

@@ +577,5 @@
> +            *vp = args.rval();
> +            return ok;
> +        }
> +
> +        /* Otherwise it's a PropertyOp-based property.  XXX handle this! */

How is this going to work with PropertyOp properties?  PropDesc doesn't seem to have a way to represent them.

Comment 13

[Brian Hackett [Laid off!]]

Comment on attachment 617135 [details] [diff] [review]
Implement much of [[SetP]] for elements

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

::: js/src/vm/ObjectImpl.cpp
@@ +674,5 @@
> +
> +    /* Convert the value being set to the element type. */
> +    double d;
> +    if (v.isDouble()) {
> +        d = v.toDouble();

I think these should use isNumber/toNumber, as v.isInt32() isn't handled in the case splitting below.

@@ +714,5 @@
> +               const Value &v, bool *succeeded)
> +{
> +    NEW_OBJECT_REPRESENTATION_ONLY();
> +
> +    JS_CHECK_RECURSION(cx, return false);

Ditto the comment on the previous patch.

@@ +765,5 @@
> +            break;
> +          case ArrayBufferElements:
> +            res = header.asArrayBufferElements().getOwnElement(cx, obj, index, &ownDesc);
> +            break;
> +        }

This needs to be refactored with the identical giant switch below and in the GetP patch.

@@ +797,5 @@
> +        }
> +
> +        obj = obj->getProto();
> +        if (obj)
> +            continue;

This logic seems a little screwy.  If the property is a writable data descriptor then we fall through to here, and keep walking the prototype chain?  I also don't understand why the getOwnElement switch needs to be repeated for the receiver below.  Some more comments would be helpful here, especially about the relationship between obj and receiver.

Comment 14

[Jeff Walden [:Waldo]]

Attachment: Implement much of [[SetP]] for elements

Hmm, good catch -- the existing proposed algorithm's all wrong.  I proposed a new algorithm on the es-discuss list that I think addresses all the issues, and this patch implements it:

https://mail.mozilla.org/pipermail/es-discuss/2012-April/022561.html

Because this is all still proposal-land, I'm continuing to refrain from comments.  When we have something closer to an actual spec, we can start referring to numbered steps and such to make clear that we implement the prescribed semantics.

Comment 15

[Jeff Walden [:Waldo]]

Attachment: Implement [[HasProperty]] for elements

This is rebased for changes in previous patches here, but semantically I don't think it's much different.

Comment 16

[Brian Hackett [Laid off!]]

Comment on attachment 617732 [details] [diff] [review]
Implement much of [[SetP]] for elements

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

::: js/src/vm/ObjectImpl.cpp
@@ +659,5 @@
> +        d = 0.0;
> +    } else if (v.isPrimitive()) {
> +        if (v.isString()) {
> +            if (!ToNumber(cx, v, &d))
> +                return false;

I think this can use JS_ALWAYS_TRUE (also nice to know if setting elements on typed arrays is infallible).

@@ +705,5 @@
> +        PropDesc ownDesc;
> +        if (!GetOwnElement(cx, obj, index, &ownDesc))
> +            return false;
> +
> +        if (ownDesc.isUndefined()) {

This looks like it needs a !

@@ +726,5 @@
> +                *succeeded = true;
> +                return Invoke(cx, args);
> +            }
> +
> +            if (ownDesc.isDataDescriptor()) {

It would be good if this came first, to reinforce it is the common case.

@@ +733,5 @@
> +                    return true;
> +                }
> +
> +                if (receiver == obj) {
> +                    PropDesc updateDesc; // = { [[Value]]: v }

updateDesc needs initialization, right?  Is this implementation meant to be actually functional yet?  It's also weird to have the 'receiver == obj' test when the if and else branches are identical, is this a result of missing functionality?

Comment 17

[Jeff Walden [:Waldo]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/a9764c9ec124

Re using NumberValue everywhere, I realized I could simplify this by adding NumberValue() overloads for all the fixed-size types, then those could do setInt32 and such as appropriate.  This would also have the benefit of improving any other code that calls NumberValue with more-precise types.  So I added those overloads, as well as ones for float/double, and switched everything over to NumberValue.

The recursion bits might not be necessary, removed for now.  I guess I was thinking of proxy-based recursion (which isn't implemented yet, but I'm trying to be anticipatory), but that cost we can/should foist on proxies, probably.

I added the ObjectValue(ObjectImpl&) overload.

I'm going to eventually augment PropDesc to be able to represent PropertyOp-based properties.  For now, tho (more precisely, until I get array lengths not using PropertyOps), I'm punting the issue.  I'll fix it before the representation switch happens, definitely.


https://hg.mozilla.org/integration/mozilla-inbound/rev/d8aac2bd90db

Good call on refactoring the giant switches, did that -- and added js::GetOwnElement, since doubtless it'll be needed soon too for things like Object.getOwnPropertyDescriptor.

It's actually not okay to use JS_ALWAYS_TRUE for ToNumber, because the value might be a rope string and linearization might fail.  (This is a bug in the current code.)  But moreover, since the time the original algorithm was implemented, element assignment in typed arrays was changed to use standard conversion semantics, like ToInt32 and such, so this code's all wrong already.  Given that, it doesn't seem worth changing the existing algorithm's copy to not use always-true, because it'll disappear soon.


https://hg.mozilla.org/integration/mozilla-inbound/rev/d167f7fbb53e


I also landed this:

https://hg.mozilla.org/integration/mozilla-inbound/rev/ca7b13e02cd5

which in a thinko I tagged as being a followup to bug 747197's patches, but really it's a followup to changes here.  :-\  I noted it there as well, just to be complete.


https://hg.mozilla.org/integration/mozilla-inbound/rev/718ced982de1

...and then I had to back everything out because the NumberValue overloads weren't quite working right across all architectures and compilers, and I was tired of holding up inbound.  I'll get a working fix, try it, then repush.

Comment 18

[Jeff Walden [:Waldo]]

A sprinkle of templates later to fix the NumberValue issue (without inducing compiler warnings), and things should be good now:

https://hg.mozilla.org/integration/mozilla-inbound/rev/9bab3e0dd69d
https://hg.mozilla.org/integration/mozilla-inbound/rev/e586bd795354
https://hg.mozilla.org/integration/mozilla-inbound/rev/b7a03f6f1a3e

Comment 19

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/9bab3e0dd69d
https://hg.mozilla.org/mozilla-central/rev/e586bd795354
https://hg.mozilla.org/mozilla-central/rev/b7a03f6f1a3e

Comment 20

[Jeff Walden [:Waldo]]

Warning fix (std::numeric_limits<js::uint8_clamped> is of course kind of meaningless, didn't think about that getting instantiated by this):

https://hg.mozilla.org/integration/mozilla-inbound/rev/27d42a3feef1

Comment 21

[Jeff Walden [:Waldo]]

At this point most of the new elements stuff is in place.  There's a lot of stubbing and not-yet-implemented parts, but the outline of it is there.  Given that now I've switched over to porting over the existing property infrastructure to the new representation (no bug filed yet, but I'm getting there), this bug's either going to sit dormant for awhile or has reached the end of its usefulness.  More and smaller bugs seems better than few and larger ones, so I'm declaring this bug done.  Future elements work, including fleshing out the portions of the current representation that are unimplemented, will occur in new bugs.