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

Framework: Optimize block validation attribute testing #5911

Closed
wants to merge 2 commits into from

Conversation

aduth
Copy link
Member

@aduth aduth commented Mar 30, 2018

Related: #5897

This pull request seeks to optimize the block validator, specifically in its handling of attributes, replacing use of Array with Set, where lookup via Set#has is many magnitudes more performant than Array#indexOf (~4x faster with CoreJS shim, ~9x faster natively). To my surprise while testing, I was not aware that Set is supported in IE11. Our build process still applies CoreJS, though I'm unsure if it is applied as a proper shim (when not already present in environment). In any case, a further step may be to drop this, though this should probably already be handled via babel-preset-env.

Testing instructions:

Verify that tests pass, and there are no regressions in the behavior of block validation (including invalidation), particularly affecting "meaningful attributes" (boolean attributes like disabled or enumerated attributes like contenteditable="true").

@aduth aduth added [Feature] Block API API that allows to express the block paradigm. [Type] Performance Related to performance efforts labels Mar 30, 2018
These are never accessed directly, only ever combined into the final `MEANINGFUL_ATTRIBUTES`. While it works as-is, there may be overhead in constructing the set when an array will suffice.
@aduth
Copy link
Member Author

aduth commented Mar 30, 2018

Actually, upon a more fair consideration (smaller set size), the difference is not as noticeable. A native Set#has on a set of 10 items is still faster than Array#indexOf by about 15%, but the shimmed CoreJS Set#has is actually half as fast as Array#indexOf on the same small set of items. (jsperf)

@aduth
Copy link
Member Author

aduth commented Mar 30, 2018

Unfortunately, even in browsers where Set is supported, we're apparently still forcing the polyfill:

MEANINGFUL_ATTRIBUTES.constructor === __WEBPACK_IMPORTED_MODULE_3_babel_runtime_core_js_set__.default
// true

I think moving forward with these changes would only be worthwhile if this could be resolved.

@aduth aduth added the [Status] In Progress Tracking issues with work in progress label Mar 30, 2018
@mtias mtias added the [Type] Build Tooling Issues or PRs related to build tooling label Apr 2, 2018
@mcsf
Copy link
Contributor

mcsf commented Apr 2, 2018

I think moving forward with these changes would only be worthwhile if this could be resolved.

Agreed. Set is definitely a good step, but should we close this until then?

@aduth
Copy link
Member Author

aduth commented Apr 3, 2018

Sure, let's. In the meantime, I think I'll keep Set in #5897 as an anticipated future optimization.

@aduth aduth closed this Apr 3, 2018
@aduth aduth deleted the optimize/validation-set branch April 3, 2018 01:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Block API API that allows to express the block paradigm. [Status] In Progress Tracking issues with work in progress [Type] Build Tooling Issues or PRs related to build tooling [Type] Performance Related to performance efforts
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants