<div dir="ltr">On Thu, Mar 8, 2018 at 3:06 PM, Kris Maglione <span dir="ltr"><<a href="mailto:kmaglione@mozilla.com" target="_blank">kmaglione@mozilla.com</a>></span> wrote:<br><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><span class="gmail-">On Thu, Mar 08, 2018 at 02:40:52PM -0800, Bobby Holley wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
I've seen a lot of momentum around migrating chrome-only XPIDL interfaces<br>
to WebIDL. I'm concerned that insufficient attention is being paid to the<br>
impact on our binary size.<br>
<br>
Fundamentally, the WebIDL bindings improve performance and spec correctness<br>
at the expense of code size (and build times). This makes sense for things<br>
that are web-exposed or performance-sensitive. But since the webidl<br>
bindings are also more modern and easier to use, I'm concerned that people<br>
will use them indiscriminately for all sorts of internal APIs, and our<br>
binary will bloat by a thousand paper cuts.<br>
<br>
A WebIDL method binding can easily cost a kilobyte or more, depending on<br>
the number and types of the arguments. If we were to convert all of our<br>
existing xpidl methods, we could approach 10MB in added code size.<br>
</blockquote>
<br></span>
I'm not sure how much of an immediate concern this should be.<br>
<br>
There are different costs to WebIDL and XPIDL bindings. WebIDL bindings have more cost in terms of compiled code size. XPIDL have greater costs in terms of performance and runtime memory. I'm not sure exactly where the balance is as far as impact to package size.<br>
<br>
And I think the benefits of WebIDL interfaces apply as much to our internal uses as they do to web-exposed interfaces. The amount of WebIDL overhead I regularly see in profiles can be staggering. And XPIDL has enough foot-guns when interfacing with JS that it's easy enough cause confusion and breakage even when dealing with internal code.<br></blockquote><div><br></div><div>My point is that the cost of WebIDL bindings scales with the number of methods in the tree, whereas the cost of XPIDL bindings scale with usage. I'm totally supportive of moving frequently-called methods to WebIDL. But there's an enormous surface of rarely-used XPIDL APIs in our tree where the tradeoffs don't make sense (over five thousand methods, and several thousand more attributes). We generally need these APIs for something or other - devtools, tests, some infrequent or one-time browser operation, etc - but a few extra microseconds of call overhead is fine.<br></div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
That said, if we're worried about binary size becoming an issue for internal interfaces, there are things we can do to reduce the code size of bindings. Particularly if we're willing to eat the performance costs.<br></blockquote><div><br></div><div>WebIDL bindings are optimized for speed above all else, and that shouldn't have to change to mitigate overuse.<br></div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
At any rate, I don't expect us to convert anywhere near all of our XPIDL interfaces to WebIDL. A lot of them don't need to be exposed to JS at all. A lot of those should still go away, but they don't need WebIDL bindings, just concrete native classes. And a lot of the rest are little-enough used that I can't see anyone spending the effort on converting them.</blockquote><div><br></div><div>I am basically worried about two things:</div><div>(1) Wholescale conversions of big interfaces in the name of cleanup and ergonomics. See bug 1341546 for an example.</div><div>(2) People sticking new non-performance-critical things on WebIDL interfaces because that's where the other (possibly-performance-critical) code lives.<br></div><div> <span class="gmail-"></span><br><span class="gmail-">
</span></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><span class="gmail-"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
Gating on DOM peer review gave us some degree of oversight to prevent<br>
overuse. What should replace it?<br>
</blockquote>
<br></span>
How much have DOM peers been focusing on preventing over-use, so far? Granted, most of the WebIDL bindings I've created to date have been to address measurable performance issues, but I've never had a reviewer suggest that I should be worried about over-use.</blockquote><div><br></div><div>It hasn't been a concern because WebIDL has mostly been used for web-exposed interfaces, and the momentum to convert internal interfaces is a relatively recent trend. FWIW, I did plan to bring it up at the next DOM peer meeting.</div><div><br></div><div>I'm not entirely sure whether a review gate is necessary. But at the very least, I want to establish a consensus around that we should only use WebIDL to expose internal interfaces if one of the following applies:</div><div>(A) The API is likely to be called hundreds of times under normal browser execution.</div><div>(B) The API is associated with a DOM object, and thus adding it [ChromeOnly] to that interface is particularly convenient.</div><div>(C) The API uses complex arguments like promises that XPIDL doesn't handle in a nice way.</div><div><br></div><div>Opinions?<br></div><div><br></div><div>On Thu, Mar 8, 2018 at 3:16 PM, Stuart Philp <span dir="ltr"><<a href="mailto:sphilp@mozilla.com" target="_blank">sphilp@mozilla.com</a>></span> wrote:<br><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div><div dir="auto">Generally
 I think we’d take performance and memory wins over installer size, but 
we monitor all three and if installer size grows (gradually) by an 
uncomfortable amount we ought to make a call on the trade off. We can 
bring it to product should that happen. </div></div></blockquote></div></div></div><div><br></div><div>The problem is precisely that it's gradual - a few kilobytes at a time, certainly nothing to trigger our alerts. Waiting for it all to pile up and then launching a herculean effort to move things _back_ to XPIDL would be a huge waste of time, which is why I'm trying to address the problem now.<br></div><div> <br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div class="gmail-HOEnZb"><div class="gmail-h5">
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
On Thu, Mar 8, 2018 at 2:01 PM, Kris Maglione <<a href="mailto:kmaglione@mozilla.com" target="_blank">kmaglione@mozilla.com</a>> wrote:<br>
<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
It is now possible[1] to create chrome-only WebIDL interfaces in the<br>
dom/chrome-webidl/ directory that do not require review by DOM peers after<br>
every change. If you maintain an internal performance-sensitive XPIDL<br>
interface, or are considering creating one, I'd encourage you to consider<br>
migrating it to WebIDL.<br>
<br>
Some caveats to keep in mind:<br>
<br>
- Interfaces in this directory must be annotated with [ChromeOnly].<br>
Dictionaries, however, can be included without any special annotations.<br>
<br>
- If you are new to writing WebIDL files, I'd still encourage you to ask a<br>
DOM peer to review at least your initial check-in.<br>
<br>
- Please make sure that you do not attempt to expose any of the interface<br>
or dictionary types defined in these WebIDL files to web contexts, through<br>
interfaces defined in dom/webidl/. Doing so would require (and fail) DOM<br>
peer review, in any case, but please think ahead.<br>
<br>
Thanks.<br>
<br>
- Kris<br>
<br>
[1]: As of bugs 1443317 and 1442931<br>
</blockquote></blockquote>
</div></div></blockquote></div><br></div></div>