1278583 - Switch DOM code from using class hasInstance hooks to using Symbols.hasInstance

Status: RESOLVED FIXED

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

Comment 1

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Change the value passed to JS::OrdinaryHasInstance to be a HandleValue, not MutableHandleValue

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Switch DOM code from using hasInstance class hooks to using Symbol.hasInstance

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Expose a JS::OrdinaryHasInstance

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Switch DOM code from using hasInstance class hooks to using Symbol.hasInstance

I'm not a huge fan of this patch, actually.  It adds static methods to pretty much all DOM interfaces, and hence tons of data tables to the generated code...  Another option would be to flag the DOMIfaceAndProtoJSClass somehow in the cases when we can use dom::InterfaceHasInstance and then add manual code to property definition and Xray lookups to notice that flag and define the HasInstance function.

On a separate note, this creates separate JSFunctions for each interface object.  We _could_ try to reuse the same one for all the things using InterfaceHasInstance....

Another thing we could consider is only defining this on root interfaces, I guess.  That would help a bit with the codesize impact, but it would still be significant.

Comment 5

[Jan de Mooij [:jandem]]

Comment on attachment 8760866 [details] [diff] [review]
part 1.  Expose a JS::OrdinaryHasInstance

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

::: js/src/jsapi.h
@@ +2183,5 @@
>  JS_HasInstance(JSContext* cx, JS::Handle<JSObject*> obj, JS::Handle<JS::Value> v, bool* bp);
>  
> +namespace JS {
> +
> +// Imeplementation of http://www.ecma-international.org/ecma-262/6.0/#sec-ordinaryhasinstance

Nit: typo, implementation. Usually we also mention ES6 7.3.19

While you're here, maybe we should add a note like the one we have for JS::OrdinaryToPrimitive - people just looking for the instanceof equivalent will want JS_HasInstance instead of OrdinaryHasInstance.

::: js/src/jsfun.cpp
@@ +704,5 @@
>  /*
>   * ES6 (4-25-16) 7.3.19 OrdinaryHasInstance
>   */
>  bool
> +OrdinaryHasInstance(JSContext* cx, HandleObject objArg, MutableHandleValue v, bool* bp)

JS::OrdinaryHasInstance

Comment 6

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Expose a JS::OrdinaryHasInstance

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8760908 [details] [diff] [review]
part 1.  Expose a JS::OrdinaryHasInstance

Er, sorry.  This was just adding the "JS::" in the impl, as you asked.  No need for more review; I still need to fix the first two review comments.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Attachment: part 3. Make static stuff (static attributes, methods, constants) actually work on globals

Comment 9

[Boris Zbarsky [:bzbarsky]]

Attachment: part 4. Switch DOM code from using hasInstance class hooks to using Symbol.hasInstance

Comment 10

[Peter Van der Beken [:peterv]]

(In reply to Boris Zbarsky [:bz] (Out June 25-July 6) from comment #4)
> I'm not a huge fan of this patch, actually.  It adds static methods to
> pretty much all DOM interfaces, and hence tons of data tables to the
> generated code...  Another option would be to flag the
> DOMIfaceAndProtoJSClass somehow in the cases when we can use
> dom::InterfaceHasInstance and then add manual code to property definition
> and Xray lookups to notice that flag and define the HasInstance function.
> 
> On a separate note, this creates separate JSFunctions for each interface
> object.  We _could_ try to reuse the same one for all the things using
> InterfaceHasInstance....

Can we quantify the codesize hit a bit? I'm leaning to having the flag, but it's hard to make you do more work without being able to justify it :-). Unless you think we should just do it anyway.

Comment 11

[Boris Zbarsky [:bzbarsky]]

> Can we quantify the codesize hit a bit? 

Good question.   Nicholas, do you have a good suggested way to measure this?

Comment 12

[Nicholas Nethercote [inactive]]

(In reply to Boris Zbarsky [:bz] from comment #11)
> > Can we quantify the codesize hit a bit? 
> 
> Good question.   Nicholas, do you have a good suggested way to measure this?

Attachment 8769526 [details] is the script I've been using to make measurements. Linux-only, and you run it in $OBJDIR/dist/bin/.

Comment 13

[Boris Zbarsky [:bzbarsky]]

Thanks.  Acccording to the numbers from that:

Before, total memory for sStaticMethods (and their disablers, id lists, etc): 4656 bytes
Before, total memory for sNativeProperties: 35624 bytes

After, total memory for sStaticMethods: 97424 bytes
After, total memory for sNativeProperties: 53408 bytes

So a gain of about 110KB.  The baseline here is that the total static data is about 10MB.  So this patch would be a 1% increase or so...

I'm leaning toward not doing that.

Comment 14

[Peter Van der Beken [:peterv]]

(In reply to Boris Zbarsky [:bz] from comment #13)
> I'm leaning toward not doing that.

Not adding 1%? If so, I'd also rather have a smaller increase in static data.

Comment 15

[Boris Zbarsky [:bzbarsky]]

Attachment: part 4. Rip out the now-unused construct hook holder bits in bindings

Comment 16

[Boris Zbarsky [:bzbarsky]]

Attachment: part 5. Switch DOM code from using hasInstance class hooks to using Symbol.hasInstance

Comment 17

[Peter Van der Beken [:peterv]]

Comment on attachment 8772542 [details] [diff] [review]
part 4.  Rip out the now-unused construct hook holder bits in bindings

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

Thanks for doing this.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f46fe46b763e
part 1.  Expose a JS::OrdinaryHasInstance.  r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/cef5352014e7
part 2.  Change the value passed to JS::OrdinaryHasInstance to be a HandleValue, not MutableHandleValue.  r=jandem
https://hg.mozilla.org/integration/mozilla-inbound/rev/3ada8c92f969
part 3.  Make static stuff (static attributes, methods, constants) actually work on globals.  r=peterv
https://hg.mozilla.org/integration/mozilla-inbound/rev/291d5bf2f520
part 4.  Rip out the now-unused construct hook holder bits in bindings.  r=peterv
https://hg.mozilla.org/integration/mozilla-inbound/rev/27511736d22a
part 5.  Switch DOM code from using hasInstance class hooks to using Symbol.hasInstance.  r=peterv

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/f46fe46b763e
https://hg.mozilla.org/mozilla-central/rev/cef5352014e7
https://hg.mozilla.org/mozilla-central/rev/3ada8c92f969
https://hg.mozilla.org/mozilla-central/rev/291d5bf2f520
https://hg.mozilla.org/mozilla-central/rev/27511736d22a