Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Implement a getElementIfPresent object method

RESOLVED FIXED in mozilla10

Status

()

Core
JavaScript Engine
P2
normal
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: bz, Assigned: bz)

Tracking

7 Branch
mozilla10
x86
Mac OS X
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments)

(Assignee)

Description

6 years ago
See discussion in bug 697343.
(Assignee)

Updated

6 years ago
Depends on: 698551
(Assignee)

Comment 1

6 years ago
Created attachment 570902 [details] [diff] [review]
part 1.  Create a getElementIfPresent method on JSObject with a generic implementation and use it from JSArray code.
Attachment #570902 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 2

6 years ago
Created attachment 570903 [details] [diff] [review]
part 2.  Add an optional getElementIfPresent ObjectOps hook.
Attachment #570903 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 3

6 years ago
Created attachment 570904 [details] [diff] [review]
part 3.  Expose IndexToId to API consumers.
Attachment #570904 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 4

6 years ago
Created attachment 570905 [details] [diff] [review]
ug  part 4.  Implement a JS_GetElementIfPresent API.
Attachment #570905 - Flags: review?(jwalden+bmo)
(Assignee)

Comment 5

6 years ago
Created attachment 570907 [details] [diff] [review]
part 5.  Implement getElementIfPresent on nodelist proxies.
Attachment #570907 - Flags: review?(peterv)
(Assignee)

Updated

6 years ago
Blocks: 699237
(Assignee)

Updated

6 years ago
Priority: -- → P2
Whiteboard: [need review]
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.
Attachment #570902 - Flags: review?(jwalden+bmo) → review+
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.
Attachment #570903 - Flags: review?(jwalden+bmo) → review+
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::|.
Attachment #570904 - Flags: review?(jwalden+bmo) → review+
Attachment #570905 - Flags: review?(jwalden+bmo) → review+
(Assignee)

Comment 9

6 years ago
> File a followup to have a Debug_SetValueToCrashOnTouch?

Bug 699714.

Addressed the other comments.
(Assignee)

Updated

6 years ago
Blocks: 699705
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 :-/.
Attachment #570907 - Flags: review?(peterv) → review+
(Assignee)

Comment 11

6 years ago
> 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.
(Assignee)

Comment 12

6 years ago
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.
(Assignee)

Comment 13

6 years ago
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...
(Assignee)

Comment 14

6 years ago
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.  :(
Flags: in-testsuite-
Whiteboard: [need review]
Target Milestone: --- → mozilla10
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
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
(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

6 years ago
I was curious: how much does this speedup, say, a best-case microbenchmark?  Also, did this improve any Dromaeo scores?
(Assignee)

Comment 18

6 years ago
> 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.
(Assignee)

Comment 19

6 years ago
> bugs this bocks

Bugs this _blocks_, of course.
You need to log in before you can comment on or make changes to this bug.