Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: Expose globalObject on proxyHandler [CEReactions] methods #163

Merged

Conversation

ExE-Boss
Copy link
Contributor

@ExE-Boss ExE-Boss commented Jan 30, 2020

Needed for jsdom/jsdom#2548.


See also: #162

@ExE-Boss ExE-Boss force-pushed the fix/expose-global-object-on-proxy-handlers branch 2 times, most recently from e29c996 to 246f799 Compare January 30, 2020 23:17
@domenic
Copy link
Member

domenic commented Feb 1, 2020

Hmm, I'm probably missing something obvious, but why does this need to be done here instead of in the jsdom CEReactions processor code?

Also, the test snapshots aren't super-helpful in figuring this out, because they just declare a globalObject variable and then don't use it. Could we add a test that uses it, to illustrate when this is helpful?

Finally, I'll note that there's an alternate approach for getting per-instance state into proxy handlers, which is to make the proxy handler a class (so that methods are shared in the prototype) and then assign any per-instance state as fields on the class. We'd probably want to measure the performance of such an approach though.

@ExE-Boss
Copy link
Contributor Author

ExE-Boss commented Feb 2, 2020

The globalObject variable is used by the CEReactions steps:

const globalObject = target[impl]._globalObject;
let namedValue = V;
namedValue = conversions[\\"DOMString\\"](namedValue, {
context: \\"Failed to set the '\\" + P + \\"' property on 'CEReactions': The provided value\\"
});
CEReactions.preSteps(globalObject);
try {
const creating = !target[impl][utils.supportsPropertyName](P);
if (creating) {
target[impl][utils.namedSetNew](P, namedValue);
} else {
target[impl][utils.namedSetExisting](P, namedValue);
}
} finally {
CEReactions.postSteps(globalObject);
}

@domenic
Copy link
Member

domenic commented Feb 3, 2020

Was that an answer to my question of

Hmm, I'm probably missing something obvious, but why does this need to be done here instead of in the jsdom CEReactions processor code?

or just my question about the test snapshots? If so, I don't quite understand.

@ExE-Boss
Copy link
Contributor Author

ExE-Boss commented Feb 3, 2020

After some refactoring, I’ve made it so that there’s now per‑global proxy handlers for interfaces that use [CEReactions] on named and indexed setters and deleters.

This keeps the performance gains from #156 for the most common case (no [CEReactions] on named or indexed getters or deleters), while ensuring that #130 doesn’t break.

@domenic
Copy link
Member

domenic commented Feb 3, 2020

Given that [CEReactions] is on the majority of classes in jsdom, and definitely the majority of performance-sensitive ones, I think we should benchmark such a change before committing to it.

@pmdartus
Copy link
Member

I ran the performance benchmark in jsdom, and I was able to measure a 5-10% performance regressions when introducing this change on certain benchmarks. The benchmark is comparing the current master of jsdom against the custom elements branch + this PR. The performance improvements can probably be attributed to the create element refactor that has been done on the custom element branch.

IMO the performance regression is acceptable compared to value-added by custom element introduction (even if I am completely biased). I am counting a lot on #158 to improve the overall performance even after the introduction of this regression.

Results
Test name Master (ops/sec) CE + WeakMap (ops/sec) Delta
dom/compare-document-position/compare ancestor 20,866 16,475 -21.04%
dom/compare-document-position/compare descendant 19,536 18,565 -4.97%
dom/compare-document-position/compare siblings 901,133 898,123 -0.33%
dom/construction/createComment 1,165,566 1,301,560 11.67%
dom/construction/createDocumentFragment 1,320,087 1,331,535 0.87%
dom/construction/createElement 185,593 262,536 41.46%
dom/construction/createEvent 189,774 210,985 11.18%
dom/construction/createNodeIterator 1,225,422 1,228,464 0.25%
dom/construction/createProcessingInstruction 648,505 645,610 -0.45%
dom/construction/createTextNode 1,008,861 1,037,554 2.84%
dom/inner-html/tables 0.31 0.29 -6.45%
dom/named-properties/setAttribute 3,110 3,238 4.12%
dom/tree-modification/appendChild: many parents 22.46 19.9 -11.40%
dom/tree-modification/appendChild: many siblings 246,965 238,447 -3.45%
dom/tree-modification/appendChild: no siblings 244,536 212,991 -12.90%
dom/tree-modification/insertBefore: many siblings 207,314 192,440 -7.17%
dom/tree-modification/removeChild: many parents 187,021 192,440 2.90%
dom/tree-modification/removeChild: many siblings 127,226 142,754 12.21%
dom/tree-modification/removeChild: no siblings 106,098 115,957 9.29%
html/parsing/text 13.08 14.46 10.55%
jsdom/api/new JSDOM() defaults 249 261 4.82%
jsdom/api/new JSDOM() with many elements 25.36 24.38 -3.86%

@domenic
Copy link
Member

domenic commented Feb 15, 2020

I went to merge this but I'm not sure I can write an accurate commit message. Would the following be an accurate summary of the changes?

Put globalObject into scope for processCEReactions replacement code

Now the replacement code returned by the processCEReactions handler can reference the globalObject variable, as is documented in the README. This was meant to be introduced in fda810e8d36fe64612bba533228b1b4e91f1a569, but the previous merging of 7c8037ffd4afcff01fd4ac48423675aa64aa1cea prevented that from working properly.

I wonder, is interfaceName (also documented in the readme) properly in scope still?

@ExE-Boss
Copy link
Contributor Author

Since interfaceName is a constant shared across all globals, I’ve moved it into the enclosing module scope.

@domenic domenic merged commit bfcd994 into jsdom:master Feb 16, 2020
@domenic domenic deleted the fix/expose-global-object-on-proxy-handlers branch February 16, 2020 06:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants