Closed
Bug 1452786
Opened 6 years ago
Closed 6 years ago
Stop using a chromeonly static for Foo.isInstance
Categories
(Core :: DOM: Bindings (WebIDL), enhancement, P2)
Core
DOM: Bindings (WebIDL)
Tracking
()
RESOLVED
FIXED
mozilla61
Tracking | Status | |
---|---|---|
firefox61 | --- | fixed |
People
(Reporter: bzbarsky, Assigned: bzbarsky)
References
(Blocks 1 open bug)
Details
Attachments
(2 files)
4.81 KB,
patch
|
qdot
:
review+
|
Details | Diff | Splinter Review |
16.66 KB,
patch
|
qdot
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•6 years ago
|
||
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•6 years ago
|
||
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.
Assignee | ||
Comment 3•6 years ago
|
||
> 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!
Assignee | ||
Comment 4•6 years ago
|
||
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
Attachment #8966452 -
Flags: review?(kyle)
Assignee | ||
Comment 5•6 years ago
|
||
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
Attachment #8966453 -
Flags: review?(kyle)
Updated•6 years ago
|
Attachment #8966452 -
Flags: review?(kyle) → review+
Updated•6 years ago
|
Attachment #8966453 -
Flags: review?(kyle) → review+
Pushed by bzbarsky@mozilla.com: 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
Updated•6 years ago
|
Status: NEW → ASSIGNED
Priority: -- → P2
Comment 7•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/f87a1de08a8a https://hg.mozilla.org/mozilla-central/rev/3406e123d279
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox61:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Assignee | ||
Updated•6 years ago
|
Component: DOM → DOM: Bindings (WebIDL)
You need to log in
before you can comment on or make changes to this bug.
Description
•