572577 - Internal iteration rough edges: use AutoValueVector rather than JSIdArray or pointer/length arguments

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Jeff Walden [:Waldo]]

JSIdArray is a plain-vanilla C struct, it requires manual memory management, it's harder to work with than normal vectors (including lacking bounds-checking asserts), etc.  Use AutoValueVector in APIs as far as is possible, converting to other stuff like JSIdArray as late as possible (and not using things like pointer/length argument pairs when it's unnecessary).

These changes should make implementation of Object.getOwnPropertyNames generally more pleasant and less baroque.

Three patches for all this to follow.

Comment 1

[Jeff Walden [:Waldo]]

Attachment: Patch 1: Convert non-proxy/wrapper iteration APIs to work in terms of vectors and not idarrays

Comment 2

[Andreas Gal :gal]

Nice!

Comment 3

[Jeff Walden [:Waldo]]

Attachment: Patch 2: convert proxy/wrapper code to use vectors rather than idarrays

Patch 1 doesn't depend on this patch to build, for what it's worth -- just split to keep from bloating patch 1 overfar.

Comment 4

[Jeff Walden [:Waldo]]

Attachment: Patch 3: pass vector rather than pointer/length to NativeIterator::allocate

Comment 5

[Andreas Gal :gal]

Comment on attachment 451756 [details] [diff] [review]
Patch 1: Convert non-proxy/wrapper iteration APIs to work in terms of vectors and not idarrays

Pretty.

Comment 6

[Andreas Gal :gal]

Comment on attachment 451757 [details] [diff] [review]
Patch 2: convert proxy/wrapper code to use vectors rather than idarrays

You are making enumerateOwn a bit slower by copying (instead of in-place), but I don't think it matters much. What gives me pause is using the AutoValueVector though in a place where we have ids. sizeof(id) < sizeof(jsval) in Luke's world. I think we need an AutoIdVector. Luke?

Comment 7

[Brendan Eich [:brendan]]

We are performance hackers, bean-counters -- almost everything (incluiding ES5's new APIs) gets perf scrutiny, if not from developers (think proxies being heavily used in a few years) then from bench-marketeers.

Making a vector allocation and copying to separate disjoint types or (more than justified) avoid lifetime assumptions is one thing -- that I can see if there's no way to unify types and lifetimes. Here, however:

+    if (!GetPropertyNames(cx, obj, JSITER_OWNONLY, props) || !VectorToIdArray(cx, props, &ida))

the allocation and copy are gratuitous, and as Andreas noted the types don't match (with fatvals coming).

Can we have a jsvector of jsids that is as sweet to use as AutoValueVector, and just use it throughout?

/be

Comment 8

[Jeff Walden [:Waldo]]

Oh, I see what you mean about copy/in-place.  I can change that if desired; it felt just a bit fugly to me originally, but it might make sense.

Agree about AutoIdVector, but baby steps.  :-)  We already assume jsid is roughly jsval, and AutoIdVector is a fairly simple followup (would touch a fair number of lines, but near-mechanically within them).

Comment 9

[Brendan Eich [:brendan]]

fatvals are upon us. Let's help 'em out by not making more jsid/jsval mixing.

/be

Comment 10

[Jeff Walden [:Waldo]]

(In reply to comment #7)
> Here, however:
> 
> +    if (!GetPropertyNames(cx, obj, JSITER_OWNONLY, props) ||
> !VectorToIdArray(cx, props, &ida))
> 
> the allocation and copy are gratuitous, and as Andreas noted the types don't
> match (with fatvals coming).

We already have an allocation and copy, they're just in slightly different places: once for the AutoValueVector in Snapshot, once for NativeIterator::allocate.  However, the extra copy Andreas notes is truly additional.


> Can we have a jsvector of jsids that is as sweet to use as AutoValueVector, and
> just use it throughout?

AutoIdVector is nice; getting it to interconvert easily with JSIdArray seems rather tricky, however.


(In reply to comment #9)
> fatvals are upon us. Let's help 'em out by not making more jsid/jsval mixing.

Quick followup then?  Adding it equates to a decent-ish hunk in jscntxt.h that was way outside the original laser focus of changes here.  :-)

Comment 11

[Brendan Eich [:brendan]]

(In reply to comment #10)
> (In reply to comment #7)
> We already have an allocation and copy, they're just in slightly different
> places: once for the AutoValueVector in Snapshot, once for
> NativeIterator::allocate.  However, the extra copy Andreas notes is truly
> additional.

I know; that's the one I'm calling out, too.

Making only one allocation: bonus! Three: regress!

> AutoIdVector is nice; getting it to interconvert easily with JSIdArray seems
> rather tricky, however.

Get rid of JSIdArray, then? It's recent-ish API. Or make the API lusers pay the copy and the VM never pay by having the VM use only AutoIdVector or a heap-based version of same.

> (In reply to comment #9)
> > fatvals are upon us. Let's help 'em out by not making more jsid/jsval mixing.
> 
> Quick followup then?  Adding it equates to a decent-ish hunk in jscntxt.h that
> was way outside the original laser focus of changes here.  :-)

Followup fine.

/be

Comment 12

[Jeff Walden [:Waldo]]

Attachment: Patch 4: js::AutoIdVectorize the world of enumeration and such

...and the followup.

Comment 13

[Andreas Gal :gal]

Comment on attachment 451757 [details] [diff] [review]
Patch 2: convert proxy/wrapper code to use vectors rather than idarrays

Follow-up patch fixes the jsid issue.

Comment 14

[Andreas Gal :gal]

Comment on attachment 451785 [details] [diff] [review]
Patch 4: js::AutoIdVectorize the world of enumeration and such

Looks good. We still do the extra copy + allocation but I honestly don't think it will hurt a lot. Maybe a follow-up bug with less priority? I do like the cleanup.

Comment 15

[Brendan Eich [:brendan]]

Comment on attachment 451785 [details] [diff] [review]
Patch 4: js::AutoIdVectorize the world of enumeration and such

This is very nice -- thanks. Only one nit:

>+class AutoIdVector : private AutoGCRooter
>+{
>+  public:
>+    explicit AutoIdVector(JSContext *cx
>+                          JS_GUARD_OBJECT_NOTIFIER_PARAM)
>+        : AutoGCRooter(cx, IDVECTOR), vector(cx)

Half-indent the : -- same as case and goto labels, done elsewhere.

/be

Comment 16

[Luke Wagner [:luke]]

Thanks for all the fatval consideration in the comments above!

Not coincidentally I added the same AutoIdVector, so that's great stuff.

Now, as for the changes in jsiter, I would appreciate if you touched as little of the Snapshot --> Enumerate path as possible.  I had to do some real surgery to either generate an array of jsids or array of jsvals (depending on whether this was a for-in or for-each loop) and the syntactic changes in the patch will mostly conflict.  So for now, perhaps you could continue to let the jsval/jsid incest continue unabated?

Comment 17

[Jeff Walden [:Waldo]]

Luke sez he doesn't want introduction of AutoIdVector to the iteration bits, but he would be fine with it in the proxy bits.  I tend to think they're closely intertwined, so if the choice is between patch 4 minus the iteration changes and nothing, nothing seems better to me.  Luke seemed to think this was fine (or at the very least didn't object when I said that was what I was going to do).  Bring on the fatvals, and then we can get back to this, will file the followup bug after landing everything sometime in the next couple hours...

Comment 18

[Andreas Gal :gal]

No objections to landing parts of the patches only as look requested. Very nice cleanup. Thanks!

Comment 19

[Jeff Walden [:Waldo]]

Attachment: Patch 5: back to filtering proxied properties in-place

This is a little more complicated and hearkening back to the previous state, but it does have the improvement (in clarity and efficiency) that it doesn't always copy the particular id from higher location to lower location even if no copy need be done (index increment distinguished an effective copy from an ineffective one, which felt a little obscure to me).  Put another way, my spider sense doesn't tingle over this formulation like it did over the latter.  :-)

Comment 20

[Brendan Eich [:brendan]]

http://hg.mozilla.org/tracemonkey/rev/b4154838c639 has JS_LIKELY but it should not be needed there -- prefetched is predicted and the miss case is a target in the pipe, so a minor penalty.

We should be sparing in JS_LIKELY usage. This one is truly well-used -- the IE botch of allowing for (i in null); is in ES5 but no one writes such loops or wants them to perform micro-optimally -- but still unnecessary.

The inability to pass boolish args to JS_LIKELY is a pain, to boot. More reason to avoid it if it's not necessary.

/be

Comment 21

[Andreas Gal :gal]

Comment on attachment 452155 [details] [diff] [review]
Patch 5: back to filtering proxied properties in-place

Cool.

Comment 22

[Jeff Walden [:Waldo]]

http://hg.mozilla.org/tracemonkey/rev/685c840dee4b
http://hg.mozilla.org/tracemonkey/rev/f7c575acdee2
http://hg.mozilla.org/tracemonkey/rev/de325b61d17d
http://hg.mozilla.org/tracemonkey/rev/b4154838c639
http://hg.mozilla.org/tracemonkey/rev/b40124475626
http://hg.mozilla.org/tracemonkey/rev/27df663a0ab5

Comment 23

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/685c840dee4b