Optimize block parser by avoiding repeated HTML parsing and matcher creation #39424
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
jsnajdr
added
the
[Feature] Parsing
|
Size Change: +293 B (0%) Total Size: 1.16 MB
ℹ️ View Unchanged
|
jsnajdr
force-pushed
the
optimize/block-parser
branch
from
4d3ad0e
to
dc4ae2e
Compare
youknowriad
reviewed
Mar 14, 2022
| * | ||
| * @return {Function} The same function memoized. | ||
| */ | ||
| function memoize( fn ) { |
There was a problem hiding this comment.
What about using memoize which is already used in our code base?
There was a problem hiding this comment.
You're right, memize would work here, too. I'll migrate to that.
There was a problem hiding this comment.
Migrated the memoization to memize in ba1812d.
| * @return {Node} Parsed DOM node. | ||
| */ | ||
| function parseHtml( innerHTML ) { | ||
| return hpqParse( innerHTML, ( h ) => h ); |
There was a problem hiding this comment.
This is a bit weird, it seems we're using hpq not in the way it was supposed to be used. Makes me wonder if there's some internal optimization to be made. Isn't this just the same thing as returning innerHTML?
There was a problem hiding this comment.
innerHTML is a string, while hpq( html, identity ) returns the parsed Node object. A usual matcher would select something from the DOM node (typically some HTML attribute or text node), but an identity matcher that just returns the parsed node works, too.
|
The performance test don't seem to indicate improvement here, do you think this is within the error margin? |
The metric affected should be mainly It should be more visible when looking at long term trends, e.g., at |
Yeah, I've been procrastinating the work on that. We probably need more eye on this, I'm happy to share my repo. We may have reached the limit of the free plan in planetscale.com |
Yeah I'd be happy to contribute here 👍 |
jsnajdr
force-pushed
the
optimize/block-parser
branch
from
dc4ae2e
to
ba1812d
Compare
youknowriad
reviewed
Mar 16, 2022
| source: 'attribute', | ||
| selector: '[data-custom-class-name] > *', | ||
| attribute: 'class', | ||
| }; |
There was a problem hiding this comment.
Should we do the same for the "style" attribute which is also a common one shared by a lot of blocks?
There was a problem hiding this comment.
I don't see the style attribute being read with this method anywhere. It's usually stored in the block comment (<!-- wp:heading {"style"} -->), not in the block HTML markup, is that right? Then the code path is different and doesn't involve HTML parsing and matching.
There was a problem hiding this comment.
Makes sense, so we only care about "memoizing" attribute definitions if the attribute is parsed from content. I wonder if there's a way to surface this for block authors. In documentation at least.
There was a problem hiding this comment.
For block authors, I think this matters only in rare extreme cases. Like when the block has many deprecated versions with shared attribute definitions. For example, the core/gallery block has six deprecated versions, and they share definitions for attributes like img[data-full-url]. But deduplicating them would have very little impact.
Where would be the best place to document this? We can do that anyway.
There was a problem hiding this comment.
Indeed, since most block use block.json and don't add anything "common" (rarely), it's not that important. I'm also not sure how to document this, so not important.
youknowriad
approved these changes
Mar 16, 2022
jsnajdr
force-pushed
the
optimize/block-parser
branch
from
ba1812d
to
3035be7
Compare
jostnes
pushed a commit
to jostnes/gutenberg
that referenced
this pull request
Mar 23, 2022
…reation (WordPress#39424) * getBlockAttributes: parse HTML only once * matcherFromSource: memoize matchers by schema object * Use memize to memoize
ellatrix
pushed a commit
that referenced
this pull request
Aug 12, 2022
In #39424 when we added an optimization to only parse a block's innerHTML once we also changed the behavior that the innerHTML propety represented the raw HTML loaded by the parser. Instead, what we have since that change is the DOM of the parsed HTML. In this patch we're adding yet-another parameter to the bag of arguments in `getBlockAttribute()` so that the new `raw` type can read and reproduce that original source, e.g. when reading the string `1 < 0` the parsed value's `innerHTML` will be `1 < 0` even though the block's raw content was `1 < 0`.
ellatrix
pushed a commit
that referenced
this pull request
Dec 6, 2022
In #39424 when we added an optimization to only parse a block's innerHTML once we also changed the behavior that the innerHTML propety represented the raw HTML loaded by the parser. Instead, what we have since that change is the DOM of the parsed HTML. In this patch we're adding yet-another parameter to the bag of arguments in `getBlockAttribute()` so that the new `raw` type can read and reproduce that original source, e.g. when reading the string `1 < 0` the parsed value's `innerHTML` will be `1 < 0` even though the block's raw content was `1 < 0`.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Two simple optimizations of the block parser that lead to significant speed improvement. The large-post.html sample post (1000+ blocks) took 600ms to parse before this patch, now it takes only 340ms.
getBlockAttributesparses HTML only once: before this patch, each innergetBlockAttributecall parsed the block HTML again before selecting a target value from the parsed DOM. This patch optimizes that to parse only once. Makes use of the fact thathpq()is an identity when called with an already parsed DOM nodematcherFromSourcethat converts aschemaobject to a matcher function used to be called 3900 times on thelarge-post.html, although there are only 41 unique attribute schemas. I speeded up the parser by memoizing by theschemaobject argument. There are two places where I extracted a schema to a separate variable to make it referentially unique. After this memoization, there are only 75 unique schemas. The difference between 41 and 75 is caused by some identical schemas to be defined multiple times, for example, inblock.jsonfiles.