698495 - Implement a getElementIfPresent object method

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P2)

Description

[Boris Zbarsky [:bzbarsky]]

See discussion in bug 697343.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Create a getElementIfPresent method on JSObject with a generic implementation and use it from JSArray code.

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Add an optional getElementIfPresent ObjectOps hook.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Expose IndexToId to API consumers.

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: ug part 4. Implement a JS_GetElementIfPresent API.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: part 5. Implement getElementIfPresent on nodelist proxies.

Comment 6

[Jeff Walden [:Waldo]]

Comment on attachment 570902 [details] [diff] [review]
part 1.  Create a getElementIfPresent method on JSObject with a generic implementation and use it from JSArray code.

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

::: js/src/jsarray.cpp
@@ +384,5 @@
> +        vp->setUndefined();
> +    } else {
> +        if (!obj->getGeneric(cx, idr.id(), vp))
> +            return JS_FALSE;
> +        *hole = JS_FALSE;

Total nitpickery, but let's set vp and hole in the same order in both arms of this |if|.  I don't care which order, just let's be consistent.

@@ +403,5 @@
> +
> +    return true;
> +}
> +
> +template<typename index_type>

We use CamelCaps naming for template parameters that aren't K/V single-letter affairs, so IndexType.

::: js/src/jsobj.h
@@ +1375,5 @@
>                                js::Value *vp);
>      inline JSBool getElement(JSContext *cx, JSObject *receiver, uint32 index, js::Value *vp);
> +    /* If element is not present (e.g. array hole) *present is set to
> +       false and the contents of *vp are not defined to be anything in
> +       particular. */

"unusable garbage" seems a stronger descriptor, with clearer don't-do-this overtones, than "not defined to be anything in particular".

::: js/src/jsobjinlines.h
@@ +1271,5 @@
> +        return false;
> +
> +    if (!prop) {
> +        *present = false;
> +        js::Debug_SetValueRangeToCrashOnTouch(vp, 1);

File a followup to have a Debug_SetValueToCrashOnTouch?  Setting a length-one range to crash is a bit cute.

Comment 7

[Jeff Walden [:Waldo]]

Comment on attachment 570903 [details] [diff] [review]
part 2.  Add an optional getElementIfPresent ObjectOps hook.

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

::: js/src/jsclass.h
@@ +314,5 @@
>      DefineSpecialOp     defineSpecial;
>      GenericIdOp         getGeneric;
>      PropertyIdOp        getProperty;
>      ElementIdOp         getElement;
> +    ElementIfPresentOp  getElementIfPresent; /* Can be null */

Don't capitalize sentence fragments.

Comment 8

[Jeff Walden [:Waldo]]

Comment on attachment 570904 [details] [diff] [review]
part 3.  Expose IndexToId to API consumers.

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

::: js/src/jsapi.cpp
@@ +6299,5 @@
> +
> +JS_PUBLIC_API(JSBool)
> +JS_IndexToId(JSContext *cx, uint32 index, jsid *id)
> +{
> +    return js::IndexToId(cx, index, id);

If we're inside a |using namespace js| as I think we are, nix the |js::|.

Comment 9

[Boris Zbarsky [:bzbarsky]]

> File a followup to have a Debug_SetValueToCrashOnTouch?

Bug 699714.

Addressed the other comments.

Comment 10

[Peter Van der Beken [:peterv]]

Comment on attachment 570907 [details] [diff] [review]
part 5.  Implement getElementIfPresent on nodelist proxies.

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

::: js/xpconnect/src/dombindings.cpp
@@ +1050,5 @@
> +    if (hasIndexGetter) {
> +        IndexGetterType result;
> +        *present = getItemAt(getListObject(proxy), index, result);
> +        if (*present)
> +            return Wrap(cx, proxy, result, vp);

getOwnPropertyDescriptor(...), hasOwn(...) and get(...) all return if getItemAt returns false, and don't try to get expandos or things from the prototype. I don't remember exactly if it was a conscious decision (might have been to be compatible with the existing code), but it seems like we should be consistent. If you think we should change the other methods then we should patch them, but we might be running out of time :-/.

Comment 11

[Boris Zbarsky [:bzbarsky]]

> I don't remember exactly if it was a conscious decision (might have been to be compatible > with the existing code), but it seems like we should be consistent.

Hrm.  So this testcase:

      Object.prototype[1000] = "PASS";
      alert(document.getElementsByTagName("p")[1000]);

alerts "PASS" with the old code.

Per current spec at http://www.w3.org/TR/domcore/#interface-htmlcollection and http://dev.w3.org/2006/webapi/WebIDL/#getownproperty that should also alert "PASS", I think.

I believe there was talk for a bit of doing it some other way, but then that got changed in webidl?

But you're right that we should be consistent.  I'll file a followup on this and remove that code from this patch.

Comment 12

[Boris Zbarsky [:bzbarsky]]

More precisely, I will add this code:

        vp->setUndefined();
        return true;

right after the |if (*present)| early return, still inside the hasIndexGetter block.

Oh, and not getting _expandos_ is correct per, I believe; maybe that's the confusion there.  But we do want to chain to the prototype.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Er, no.  Looks like the spec has changed again so that we do want to expose expandos.

I wonder whether we have any tests for all this...

Comment 14

[Boris Zbarsky [:bzbarsky]]

I filed bug 699826.

Pushed those patches:

https://hg.mozilla.org/integration/mozilla-inbound/rev/89565d208d94
https://hg.mozilla.org/integration/mozilla-inbound/rev/65bd02af3fdc
https://hg.mozilla.org/integration/mozilla-inbound/rev/1cfc9bc667f8
https://hg.mozilla.org/integration/mozilla-inbound/rev/72b103262305
https://hg.mozilla.org/integration/mozilla-inbound/rev/37f7444b17da

but that broke the Win64 build because the definitions of uint32 in JS and NSPR are not compatible there (unsigned __int32 in JS vs unsigned long in NSPR).  I fixed that by changing the declaration in jsproxy.h to use JSUint32 directly:

https://hg.mozilla.org/integration/mozilla-inbound/rev/6d1311691bea

We really need to sort out this integer types crap.  :(

Comment 15

[Marco Bonardo [:mak]]

https://hg.mozilla.org/mozilla-central/rev/89565d208d94
https://hg.mozilla.org/mozilla-central/rev/65bd02af3fdc
https://hg.mozilla.org/mozilla-central/rev/1cfc9bc667f8
https://hg.mozilla.org/mozilla-central/rev/72b103262305
https://hg.mozilla.org/mozilla-central/rev/37f7444b17da
https://hg.mozilla.org/mozilla-central/rev/6d1311691bea

Comment 16

[Cameron McCormack (:heycam)]

(In reply to Boris Zbarsky (:bz) from comment #11)
> Hrm.  So this testcase:
> 
>       Object.prototype[1000] = "PASS";
>       alert(document.getElementsByTagName("p")[1000]);
> 
> alerts "PASS" with the old code.
> 
> Per current spec at http://www.w3.org/TR/domcore/#interface-htmlcollection
> and http://dev.w3.org/2006/webapi/WebIDL/#getownproperty that should also
> alert "PASS", I think.

Confirming that that's what the spec requires.

> I believe there was talk for a bit of doing it some other way, but then that
> got changed in webidl?

There was talk of that (for simplicity), but it never solidified and the spec didn't change.

Comment 17

[Luke Wagner [:luke]]

I was curious: how much does this speedup, say, a best-case microbenchmark?  Also, did this improve any Dromaeo scores?

Comment 18

[Boris Zbarsky [:bzbarsky]]

> I was curious: how much does this speedup, say, a best-case microbenchmark?

It sped up the nearly-best-case microbenchmarks in the bugs this bocks by at least 30%, iirc.

> Also, did this improve any Dromaeo scores?

Not as far as I know.  It improved some Peacekeeper scores, though.

Comment 19

[Boris Zbarsky [:bzbarsky]]

> bugs this bocks

Bugs this _blocks_, of course.