686582 - Use element-valued ObjectOps methods

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jeff Walden [:Waldo]]

Attachment: Add element-valued methods to JSObject

Keep on splitting...

Comment 1

[Jeff Walden [:Waldo]]

Attachment: Specialize the lookupElement implementations a bit

I'm going to specialize each ObjectOps method in a separate patch, for more bite-sizedness.

Comment 2

[Jeff Walden [:Waldo]]

Attachment: Make the JSON Walk algorithm use getElement, not getProperty, for a numeric loop

Comment 3

[Jeff Walden [:Waldo]]

Attachment: Specialize getElement

Comment 4

[Jeff Walden [:Waldo]]

Attachment: getElementAttributes

Comment 5

[Jeff Walden [:Waldo]]

Attachment: setElementAttributes

Comment 6

[Jeff Walden [:Waldo]]

Attachment: deleteElement

Comment 7

[Jeff Walden [:Waldo]]

Attachment: defineElement

Comment 8

[Jeff Walden [:Waldo]]

Attachment: setElement

Comment 9

[Jeff Walden [:Waldo]]

Attachment: Use the element-specific methods a bunch more pretty-obvious places

Comment 10

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 560058 [details] [diff] [review]
Specialize the lookupElement implementations a bit

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

::: js/src/jstypedarray.cpp
@@ +632,5 @@
> +    JSObject *tarray = getTypedArray(obj);
> +    JS_ASSERT(tarray);
> +
> +    if (index < getLength(tarray)) {
> +        *propp = (JSProperty *) 1;  /* non-null to indicate found */

Could we get a #define or constant for this semi-magic value?

@@ +640,5 @@
> +
> +    if (JSObject *proto = obj->getProto())
> +        return proto->lookupElement(cx, index, objp, propp);
> +
> +    *objp = NULL;

This pattern comes up four times, verbatim, in this patch - haven't read the others yet but if it comes up more you might want to consider LookupElementOnProto or something

::: js/src/jsxml.cpp
@@ +4737,4 @@
>  xml_lookupElement(JSContext *cx, JSObject *obj, uint32 index, JSObject **objp,
>                    JSProperty **propp)
>  {
> +    JSXML *xml = reinterpret_cast<JSXML *>(obj->getPrivate());

bonus points for adding getXML()?

Comment 11

[Jeff Walden [:Waldo]]

The first three:

https://hg.mozilla.org/integration/mozilla-inbound/rev/c010d17e5d4c
https://hg.mozilla.org/integration/mozilla-inbound/rev/26ba8b0c1bdd
https://hg.mozilla.org/integration/mozilla-inbound/rev/2219fef51526

I added a constant for the JSProperty thing.  The pattern thing, unsure about yet -- will keep it in mind and do it sometime soon if it seems to make sense.  The getXML() thing would be a massive sidetrack (that pattern's in jsxml.cpp dozens of times), so I'll leave it for a different bug, and/or maybe not do it simply because touching jsxml.cpp unnecessarily presents no win.  :-\

Still more to go: don't close this bug yet, merge-monkeys!

Comment 12

[David Anderson [:dvander] - inactive, e-mail if emergency]

Comment on attachment 560065 [details] [diff] [review]
setElement

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

::: js/src/jsarray.cpp
@@ +946,5 @@
> +    } while (false);
> +
> +    if (!obj->makeDenseArraySlow(cx))
> +        return false;
> +    return js_SetPropertyHelper(cx, obj, id, 0, vp, strict);

it seems like all this logic is duplicated verbatim with array_setProperty. Is the plan to common this out, or remove it from setProperty?

Comment 13

[Jeff Walden [:Waldo]]

(In reply to David Anderson [:dvander] from comment #12)
> it seems like all this logic is duplicated verbatim with array_setProperty.
> Is the plan to common this out, or remove it from setProperty?

The plan is that the element and property paths will have entirely different implementations when the actual sparse/dense split is implemented.  Having duplication like this is just a temporary midpoint (hence the funneling into the old jsid-based APIs when trickier cases occur, in all these patches), useful to make progress on having finer-grained APIs whose implementations can later be switched around.

https://hg.mozilla.org/integration/mozilla-inbound/rev/ac7511f7bbc1
https://hg.mozilla.org/integration/mozilla-inbound/rev/0e57034d1280
https://hg.mozilla.org/integration/mozilla-inbound/rev/95f1c1855dd3
https://hg.mozilla.org/integration/mozilla-inbound/rev/5df430079b06
https://hg.mozilla.org/integration/mozilla-inbound/rev/4e647045ee7b
https://hg.mozilla.org/integration/mozilla-inbound/rev/8ddc354ef2a7
https://hg.mozilla.org/integration/mozilla-inbound/rev/8bea3e0b8cea

And that rounds out the patches here, so merge-monkeys should now feel free to mark this bug fixed at the appropriate time.  :-)

Comment 14

[(no longer active)]

https://hg.mozilla.org/mozilla-central/rev/c010d17e5d4c
https://hg.mozilla.org/mozilla-central/rev/26ba8b0c1bdd
https://hg.mozilla.org/mozilla-central/rev/2219fef51526

Comment 15

[(no longer active)]

Also:

https://hg.mozilla.org/mozilla-central/rev/ac7511f7bbc1
https://hg.mozilla.org/mozilla-central/rev/0e57034d1280
https://hg.mozilla.org/mozilla-central/rev/95f1c1855dd3
https://hg.mozilla.org/mozilla-central/rev/5df430079b06
https://hg.mozilla.org/mozilla-central/rev/4e647045ee7b
https://hg.mozilla.org/mozilla-central/rev/8ddc354ef2a7
https://hg.mozilla.org/mozilla-central/rev/8bea3e0b8cea