Last Comment Bug 686582 - Use element-valued ObjectOps methods
: Use element-valued ObjectOps methods
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla9
Assigned To: Jeff Walden [:Waldo] (remove +bmo to email)
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2011-09-13 16:16 PDT by Jeff Walden [:Waldo] (remove +bmo to email)
Modified: 2011-09-16 10:59 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Add element-valued methods to JSObject (5.20 KB, patch)
2011-09-13 16:16 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
dvander: review+
Details | Diff | Review
Specialize the lookupElement implementations a bit (6.37 KB, patch)
2011-09-13 16:17 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
dvander: review+
Details | Diff | Review
Make the JSON Walk algorithm use getElement, not getProperty, for a numeric loop (2.89 KB, patch)
2011-09-13 16:18 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
dvander: review+
Details | Diff | Review
Specialize getElement (4.45 KB, patch)
2011-09-13 16:19 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
dvander: review+
Details | Diff | Review
getElementAttributes (3.83 KB, patch)
2011-09-13 16:19 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
dvander: review+
Details | Diff | Review
setElementAttributes (4.56 KB, patch)
2011-09-13 16:20 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
dvander: review+
Details | Diff | Review
deleteElement (5.89 KB, patch)
2011-09-13 16:22 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
dvander: review+
Details | Diff | Review
defineElement (5.70 KB, patch)
2011-09-13 16:22 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
dvander: review+
Details | Diff | Review
setElement (9.26 KB, patch)
2011-09-13 16:24 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
dvander: review+
Details | Diff | Review
Use the element-specific methods a bunch more pretty-obvious places (7.41 KB, patch)
2011-09-13 16:25 PDT, Jeff Walden [:Waldo] (remove +bmo to email)
dvander: review+
Details | Diff | Review

Description Jeff Walden [:Waldo] (remove +bmo to email) 2011-09-13 16:16:08 PDT
Created attachment 560057 [details] [diff] [review]
Add element-valued methods to JSObject

Keep on splitting...
Comment 1 Jeff Walden [:Waldo] (remove +bmo to email) 2011-09-13 16:17:11 PDT
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.
Comment 2 Jeff Walden [:Waldo] (remove +bmo to email) 2011-09-13 16:18:31 PDT
Created attachment 560059 [details] [diff] [review]
Make the JSON Walk algorithm use getElement, not getProperty, for a numeric loop
Comment 3 Jeff Walden [:Waldo] (remove +bmo to email) 2011-09-13 16:19:13 PDT
Created attachment 560060 [details] [diff] [review]
Specialize getElement
Comment 4 Jeff Walden [:Waldo] (remove +bmo to email) 2011-09-13 16:19:45 PDT
Created attachment 560061 [details] [diff] [review]
getElementAttributes
Comment 5 Jeff Walden [:Waldo] (remove +bmo to email) 2011-09-13 16:20:19 PDT
Created attachment 560062 [details] [diff] [review]
setElementAttributes
Comment 6 Jeff Walden [:Waldo] (remove +bmo to email) 2011-09-13 16:22:27 PDT
Created attachment 560063 [details] [diff] [review]
deleteElement
Comment 7 Jeff Walden [:Waldo] (remove +bmo to email) 2011-09-13 16:22:59 PDT
Created attachment 560064 [details] [diff] [review]
defineElement
Comment 8 Jeff Walden [:Waldo] (remove +bmo to email) 2011-09-13 16:24:06 PDT
Created attachment 560065 [details] [diff] [review]
setElement
Comment 9 Jeff Walden [:Waldo] (remove +bmo to email) 2011-09-13 16:25:02 PDT
Created attachment 560067 [details] [diff] [review]
Use the element-specific methods a bunch more pretty-obvious places
Comment 10 David Anderson [:dvander] 2011-09-15 10:35:17 PDT
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] (remove +bmo to email) 2011-09-15 11:57:03 PDT
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] 2011-09-15 13:55:02 PDT
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] (remove +bmo to email) 2011-09-15 16:42:29 PDT
(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.  :-)

Note You need to log in before you can comment on or make changes to this bug.