Closed Bug 686582 Opened 8 years ago Closed 8 years ago

Use element-valued ObjectOps methods

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla9

People

(Reporter: Waldo, Assigned: Waldo)

Details

Attachments

(10 files)

Keep on splitting...
Attachment #560057 - Flags: review?(dvander)
I'm going to specialize each ObjectOps method in a separate patch, for more bite-sizedness.
Attachment #560058 - Flags: review?(dvander)
Attachment #560060 - Flags: review?(dvander)
Attachment #560061 - Flags: review?(dvander)
Attachment #560062 - Flags: review?(dvander)
Attached patch deleteElementSplinter Review
Attachment #560063 - Flags: review?(dvander)
Attached patch defineElementSplinter Review
Attachment #560064 - Flags: review?(dvander)
Attached patch setElementSplinter Review
Attachment #560065 - Flags: review?(dvander)
Attachment #560057 - Flags: review?(dvander) → review+
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()?
Attachment #560058 - Flags: review?(dvander) → review+
Attachment #560059 - Flags: review?(dvander) → review+
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!
Attachment #560060 - Flags: review?(dvander) → review+
Attachment #560061 - Flags: review?(dvander) → review+
Attachment #560062 - Flags: review?(dvander) → review+
Attachment #560063 - Flags: review?(dvander) → review+
Attachment #560064 - Flags: review?(dvander) → review+
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?
Attachment #560067 - Flags: review?(dvander) → review+
(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.  :-)
Whiteboard: [inbound]
Target Milestone: --- → mozilla9
Attachment #560065 - Flags: review?(dvander) → review+
You need to log in before you can comment on or make changes to this bug.