1452786 - Stop using a chromeonly static for Foo.isInstance

Status: RESOLVED FIXED

(Core :: DOM: Bindings (WebIDL), enhancement, P2)

Description

[Boris Zbarsky [:bzbarsky]]

We implemented it this way in bug 1428531, but I think we should stop doing it that way.  Specifically, if we rip it out, here's what bloaty has to say about the resulting change in libxul:

     VM SIZE                              FILE SIZE
 --------------                        --------------
  -2.1%  -113Ki LOAD [RW]              -94.2Ki  -2.0%
      -3.0% -19.5Ki .bss                         0  [ = ]
      -3.3% -94.2Ki .data.rel.ro.local     -94.2Ki  -3.3%

and that 113Ki of RW data is an appreciable fraction of our total RW data (~6MB).  Continuing work on how we represent binding data _might_ help here, but might not, because this case is a bit of a special snowflake.

So what should we do instead?  Well, I think we should just use the same function for Foo.isInstance as we do for Foo[Symbol.hasInstance].  If we ever change its behavior, at that point we can split them up into two separate functions.

Comment 1

[Boris Zbarsky [:bzbarsky]]

Actually, I guess we should use a _slightly_ separate function, since we don't really want to call on through to CallOrdinaryHasInstance and so forth.

Comment 2

[Cameron McCormack (:heycam)]

That sounds fair.  The important thing is just to retain the Foo.isInstance callers in our JS code since we know we want those ones to retain the "return true for interface objects from any global", to make it easier to change our instanceof behavior in the future.

Comment 3

[Boris Zbarsky [:bzbarsky]]

> The important thing is just to retain the Foo.isInstance callers in our JS code

Right, exactly.

I have to admit I was surprised by how much RW memory this saved... I would not have guessed nearly this much!

Comment 4

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Move the "is chrome" check for installing [ChromeOnly] stuff into the shared CreateInterfaceObjects method

Right now we do this check pretty much always anyway for isInstance... we do it
twice for anything that actually has [ChromeOnly] bits.

MozReview-Commit-ID: FHbYED4FPJe

Comment 5

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Stop using a generated chromeonly isInstance method

This changes semantics in all sorts of ways (e.g. now we get the right proto
from our |this| value instead of it being baked into the function).  But if all
our chrome callers are well-behaved this should be ok.

We _could_ bake the proto id and depth into the function itself by using
js::NewFunctionWithReserved if it were not for Xrays.  Those already need the
reserved slots on functions we Xray.

MozReview-Commit-ID: 1bYrKWWIc1P

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/f87a1de08a8a
part 1.  Move the "is chrome" check for installing [ChromeOnly] stuff into the shared CreateInterfaceObjects method.  r=qdot
https://hg.mozilla.org/integration/mozilla-inbound/rev/3406e123d279
part 2.  Stop using a generated chromeonly isInstance method.  r=qdot

Comment 7

[Eliza Balazs [:ebalazs_]]

https://hg.mozilla.org/mozilla-central/rev/f87a1de08a8a
https://hg.mozilla.org/mozilla-central/rev/3406e123d279