Constructor & prototype reform

[pmdartus]

This PR adds the capability for webidl2js to generate a brand new constructor and prototype chain per realm. Fixes #133

Note: The code change below doesn't reflect yet the proposed API changes. I want to make sure that we agree on the APIs before making all the changes.

Proposal

Interface wrapper install method introduction

The install method takes care of creating a new constructor and a new prototype chain for a realm. This method is only generated on WebIDL interfaces. This method takes a single parameter, globalObject which is the global object attached to the realm.

Interface wrapper create and createImpl signature change

Now that we have a brand new constructor and prototype per realm, we need to create wrapper instances with the right constructor. In order to do so, I am proposing passing the globalObject as the first parameter to create(globalObject, constructorArgs, privateData) and createImpl(globalObject, constructorArgs, privateData).

Interface implementation constructor signature change

In the proof-of-concept, I originally passed the globalObject to the implementation via the privateData to minimize the amount of code change in jsdom. Now thinking about it, a preferable approach making the constructor signature mirror the new create and createImpl signature: constructor(globalObject, constructorArgs, privateData).

Interface wrapper interface and expose property removal

I don't make sense to expose the shared interface wrapper class anymore.

Custom WebIDL extended attribute [WebIDL2JSFactory] removal

Not needed anymore with this proposal.

Open questions

• I am wondering if it is a good time to add support for the [Exposed] WebIDL ext attribute since JSDOM doesn't need it right? The proposed install method would blindly attach the constructor to the passed global object.

[domenic]

This all sounds reasonable to me.

Currently [Exposed] is "supported" in that the metadata is added to the output, which theoretically a consumer like jsdom could use (especially if we added Worker support). I am OK removing this support for now (with the appropriate readme updates).

Eventually we may want to move in the direction of #101 where you can do something like

require('./generated/FooInterface.js').expose("Window", windowObj);

or even

require('./generated/bundle-entry.js').exposeAllInterfaces("Window", windowObj);

But that is a separate project. So in the meantime let's just remove the expose metadata entirely and have install() (or expose()? not sure which name I like better---up to you) put it on the passed object unconditionally.

[domenic]

Interesting. I'm unsure whether this will actually be faster than just doing globalObject.DOMImplementation = { ... a bunch of generated code ... }. And it seems like it'll have issues like window.DOMImplementation.prototype.createDocument instanceof window.Function === false.

But, it seems reasonable for now though as a smaller delta from what we're doing currently...

[pmdartus]

As far as I know, creating new function every install invocation would have worse runtime performance characteristics than creating a brand new class constructor per install (like with the [WebIDL2JSFactory]).

I didn't want to make too many changes into a single PR, but my next goal is to get rid of the class and generate a property descriptor for the prototype and a property descriptor for the constructor. With this PR, the class is only used to retrieve its property descriptors. The idea would be to generate something like this in the future:

const constructorDescriptors = {
    /* generated descriptors */
};
const prototypeDescriptors = {
    /* generated descriptors */
};

function install() {
    const ctor = function Node() {
        throw new TypeError('Illegal constructor');
    };
    Object.defineProperties(ctor, constructorDescriptors);

    const proto = {};
    Object.defineProperties(proto, constructorDescriptors);

    Object.defineProperty(ctor, 'prototype', { value: proto });
    Object.defineProperty(proto, 'constructor', { value: ctor });
}

[domenic]

I'd like to not pursue any complicated attempts at sharing descriptors (and, notably, sharing the functions that are the value property of descriptors) without some benchmarks showing that's better than just creating a new copy each time. Engines are smart; I suspect that all these descriptor tricks could actually be less optimized than just

function install() {
  class Node {
    // everything goes inside here
  }
}

[pmdartus]

I am finally done updating jsdom to consume the new APIs 🎉. You can see the new APIs in actions here: jsdom/jsdom#2691

[domenic]

Mostly seems good. I'm pretty hesitant about all this copying of descriptors, and would prefer the straightforward approach that WebIDL2JSFactory used of declaring the whole class inside the install function. I suspect that will be more performant, actually; engines can deduplicate the functions behind the scenes as long as they have the same source position, and it won't involve all the reifying of property descriptors and manual re-construction of the class each time.

But, I'm more interested in getting this landed, so if this is the architecture you prefer, we can start here. Just a few comments.

[pmdartus]

I came to the realization that there is more nuance to the interface property removal from the generated wrapper. Packages like jsdom/whatwg-url rely on the interface property exposure.

The following changes are needed to accommodate the new API and keep backward compatibility:

• install the generated wrapper on a shared global object: code
• expose the generated wrapper out of the package for jsdom to install a new prototype per global object: code

After thinking more about it, making install invocation mandatory to get the wrapper class vs. exposing it via interface is the best approach. Even if the API is way less friendly, this approach prevents prototype pollution.

I wanted to raise this concern before moving forward with this PR. I would be happy to make the required changes if you think that we should bring back the interface property.

[pmdartus]

@domenic you were right the inline class declaration has the same performance characteristics or outperforms in some cases, the synthetic constructor creation. You can find the performance results below and the code here.

@TimothyGu Do you still have the code somewhere with your initial investigation (#133 (comment))? I would like to better understand why your approach didn't work in the first place.

---

Performance results

Test name	Synthetic class (ops/sec)	Inline constructor (ops/sec)	Delta
dom/compare-document-position/compare ancestor	19,712	20,731	5.17%
dom/compare-document-position/compare descendant	19,632	20,530	4.57%
dom/compare-document-position/compare siblings	1,116,531	1,132,943	1.47%
dom/construction/createComment	1,398,768	1,243,397	-11.11%
dom/construction/createDocumentFragment	1,385,996	1,366,932	-1.38%
dom/construction/createElement	200,420	203,415	1.49%
dom/construction/createEvent	193,832	197,936	2.12%
dom/construction/createNodeIterator	1,359,636	1,314,331	-3.33%
dom/construction/createProcessingInstruction	687,056	674,050	-1.89%
dom/construction/createTextNode	1,328,920	1,422,536	7.04%
dom/inner-html/tables	0.3	0.33	10.00%
dom/named-properties/setAttribute	3,404	2,828	-16.92%
dom/tree-modification/appendChild: many parents	23.41	23.33	-0.34%
dom/tree-modification/appendChild: many siblings	238,392	225,715	-5.32%
dom/tree-modification/appendChild: no siblings	216,586	213,095	-1.61%
dom/tree-modification/insertBefore: many siblings	207,314	212,118	2.32%
dom/tree-modification/removeChild: many parents	121	135	11.57%
dom/tree-modification/removeChild: many siblings	119,345	135,362	13.42%
dom/tree-modification/removeChild: no siblings	76,650	123,186	60.71%
html/parsing/text	13.07	12.59	-3.67%
jsdom/api/new JSDOM() defaults	178	231	29.78%
jsdom/api/new JSDOM() with many elements	25.33	23.97	-5.37%

[domenic]

The following changes are needed to accommodate the new API and keep backward compatibility:

Thanks for outlining this. This is indeed more subtle. I think these changes are reasonable, although I'd like to name them something like webidl2jsURL instead of URLWrapper.

[pmdartus]

Based on the performance investigation results, I decided to move forward with the inline class declaration. The PR is ready for another round of review.

[domenic]

LGTM with one question. Let's get this merged :).

[orneryd]

When will this be merged?

[domenic]

When we have free time.