Closed Bug 1278583 Opened 3 years ago Closed 3 years ago

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

Categories

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

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla51
Tracking Status
firefox50 --- wontfix
firefox51 --- fixed

People

(Reporter: bzbarsky, Assigned: bzbarsky)

References

(Blocks 1 open bug)

Details

(Whiteboard: btpp-active)

Attachments

(5 files, 4 obsolete files)

4.18 KB, patch
jandem
: review+
Details | Diff | Splinter Review
3.21 KB, patch
Details | Diff | Splinter Review
8.92 KB, patch
peterv
: review+
Details | Diff | Splinter Review
14.29 KB, patch
peterv
: review+
Details | Diff | Splinter Review
26.27 KB, patch
peterv
: review+
Details | Diff | Splinter Review
No description provided.
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Attachment #8760866 - Flags: review?(jdemooij)
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.
Attachment #8760867 - Flags: review?(peterv)
Attachment #8760864 - Attachment is obsolete: true
Attachment #8760864 - Flags: review?(jdemooij)
Attachment #8760863 - Flags: review?(jdemooij) → review+
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
Attachment #8760866 - Flags: review?(jdemooij) → review+
Attachment #8760866 - Attachment is obsolete: true
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.
Attachment #8760908 - Flags: review?(jdemooij)
Whiteboard: btpp-active
Attachment #8760867 - Attachment is obsolete: true
Attachment #8760867 - Flags: review?(peterv)
Attachment #8761447 - Flags: review?(peterv) → review+
(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.
> Can we quantify the codesize hit a bit? 

Good question.   Nicholas, do you have a good suggested way to measure this?
Flags: needinfo?(n.nethercote)
(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/.
Flags: needinfo?(n.nethercote)
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.
Flags: needinfo?(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.
Flags: needinfo?(peterv)
Attachment #8761448 - Attachment is obsolete: true
Attachment #8761448 - Flags: review?(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.
Attachment #8772542 - Flags: review?(peterv) → review+
Attachment #8772543 - Flags: review?(peterv) → review+
Pushed by bzbarsky@mozilla.com:
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
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.