957730 - set JS values more directly in bindings code

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Nathan Froyd [:froydnj]]

I am seeing size wins on ARM Android from using the
MutableValueOperations<JS::Value> methods (inherited from by
Rooted/MutableHandle), rather than the generic MutableHandle::set
method.  Some of this makes sense: e.g. setNull() instead of
set(JSVAL_NULL)) is a win, since you're not referencing a global bar.
But AFAICS, setNull() is winning (a little bit) over
set(JS::NullValue()), which one would really like the compiler to
compile identically.  Perhaps avoiding some of the generic GC bits is
the win?

Anyway, the patch is not too complex, considering it's all in
everybody's favorite function, getWrapTemplateForType.

Comment 1

[Nathan Froyd [:froydnj]]

Attachment: set JS values more directly in bindings code

This patch buys about 35K on ARM Android.  Ideally it will pass tests.

I am unsure of whether the float/double case could just use setNumber...
JS_NumberValue is just slightly different WRT setNumber, insofar as
setNumber doesn't canonicalize NaNs.  Commentary on the viability of
setNumber welcome.

Comment 2

[Nathan Froyd [:froydnj]]

(In reply to Nathan Froyd (:froydnj) from comment #0)
> I am seeing size wins on ARM Android from using the
> MutableValueOperations<JS::Value> methods (inherited from by
> Rooted/MutableHandle), rather than the generic MutableHandle::set
> method.  Some of this makes sense: e.g. setNull() instead of
> set(JSVAL_NULL)) is a win, since you're not referencing a global bar.
> But AFAICS, setNull() is winning (a little bit) over
> set(JS::NullValue()), which one would really like the compiler to
> compile identically.  Perhaps avoiding some of the generic GC bits is
> the win?

Hm, this may be because the actual setting of the value isn't getting inlined any more.  Whether that's a desirable thing or not should be left up to some benchmarking.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8357304 [details] [diff] [review]
set JS values more directly in bindings code

I think I'd prefer it if you added setUndefined(), setNull(), setString(), setObject(), setObjectOrNull(), etc convenience functions that call setValue() with the right args.  e.g. setUndefined()/setNull() would take no args at all, while setString() would just take the one arg and setObject() would take the object string and the wrapAsType.

I think that would make the code a lot cleaner at the current setValue() callsites, which are overcomplicated as it is.

> Commentary on the viability of setNumber welcome.

It's not viable right now, imo.  See discussion in bug 952687.

> this may be because the actual setting of the value isn't getting inlined any
> more.  

Hrm.  So MutableValueOperations has:

1581     void setNull() { value()->setNull(); }

and Value::setNull() is defined as:

922     void setNull() {
923         data.asBits = BUILD_JSVAL(JSVAL_TAG_NULL, 0).asBits;
924     }

I would kinda expect this to get completely inlined...  Just like I'd expect MutableHandle<JS::Value>::set() to be completely inlined....  Maybe it's just that NullValue() is in fact more expensive than what setNull() ends up doing there, somehow?

Comment 4

[Nathan Froyd [:froydnj]]

(In reply to Boris Zbarsky [:bz] from comment #3)
> I think I'd prefer it if you added setUndefined(), setNull(), setString(),
> setObject(), setObjectOrNull(), etc convenience functions that call
> setValue() with the right args.

OK, can do.

> > this may be because the actual setting of the value isn't getting inlined any
> > more.  
> 
> I would kinda expect this to get completely inlined...  Just like I'd expect
> MutableHandle<JS::Value>::set() to be completely inlined....  Maybe it's
> just that NullValue() is in fact more expensive than what setNull() ends up
> doing there, somehow?

On ARM Android (GCC 4.7 locally, haven't checked what happens on Try), it looks like ::set() isn't even inlined prior to this patch.  So I guess we're not regressing on that front?  I should see what happens on other platforms.

Comment 5

[Nathan Froyd [:froydnj]]

Attachment: set JS values more directly in bindings code

I think the helper functions added are closer to what you asked for.  I
used setUint32 instead of setNumber in hopes that future modifiers would
be less likely to use setNumber for setting doubles, given that we need
JS_NumberValue.

Things get inlined appropriately on x86-64 Linux, so I'm guessing ARM
Android is a bit of an abberation.

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8358414 [details] [diff] [review]
set JS values more directly in bindings code

Yes, setUint32 is great.

r=me

Comment 7

[Nathan Froyd [:froydnj]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/30e1dba16bb2

Comment 8

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/30e1dba16bb2