The default bug view has changed. See this FAQ.

Use element-valued ObjectOps methods

RESOLVED FIXED in mozilla9

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Waldo, Assigned: Waldo)

Tracking

unspecified
mozilla9
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(10 attachments)

5.20 KB, patch
dvander
: review+
Details | Diff | Splinter Review
6.37 KB, patch
dvander
: review+
Details | Diff | Splinter Review
2.89 KB, patch
dvander
: review+
Details | Diff | Splinter Review
4.45 KB, patch
dvander
: review+
Details | Diff | Splinter Review
3.83 KB, patch
dvander
: review+
Details | Diff | Splinter Review
4.56 KB, patch
dvander
: review+
Details | Diff | Splinter Review
5.89 KB, patch
dvander
: review+
Details | Diff | Splinter Review
5.70 KB, patch
dvander
: review+
Details | Diff | Splinter Review
9.26 KB, patch
dvander
: review+
Details | Diff | Splinter Review
7.41 KB, patch
dvander
: review+
Details | Diff | Splinter Review
Created attachment 560057 [details] [diff] [review]
Add element-valued methods to JSObject

Keep on splitting...
Attachment #560057 - Flags: review?(dvander)
Created attachment 560058 [details] [diff] [review]
Specialize the lookupElement implementations a bit

I'm going to specialize each ObjectOps method in a separate patch, for more bite-sizedness.
Attachment #560058 - Flags: review?(dvander)
Created attachment 560059 [details] [diff] [review]
Make the JSON Walk algorithm use getElement, not getProperty, for a numeric loop
Attachment #560059 - Flags: review?(dvander)
Created attachment 560060 [details] [diff] [review]
Specialize getElement
Attachment #560060 - Flags: review?(dvander)
Created attachment 560061 [details] [diff] [review]
getElementAttributes
Attachment #560061 - Flags: review?(dvander)
Created attachment 560062 [details] [diff] [review]
setElementAttributes
Attachment #560062 - Flags: review?(dvander)
Created attachment 560063 [details] [diff] [review]
deleteElement
Attachment #560063 - Flags: review?(dvander)
Created attachment 560064 [details] [diff] [review]
defineElement
Attachment #560064 - Flags: review?(dvander)
Created attachment 560065 [details] [diff] [review]
setElement
Attachment #560065 - Flags: review?(dvander)
Created attachment 560067 [details] [diff] [review]
Use the element-specific methods a bunch more pretty-obvious places
Attachment #560067 - 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
https://hg.mozilla.org/mozilla-central/rev/c010d17e5d4c
https://hg.mozilla.org/mozilla-central/rev/26ba8b0c1bdd
https://hg.mozilla.org/mozilla-central/rev/2219fef51526
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Whiteboard: [inbound]
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
Attachment #560065 - Flags: review?(dvander) → review+
You need to log in before you can comment on or make changes to this bug.