Repository navigation
Refactor to improve performance w/ weak map, hoisted regex - #13
Conversation
wooorm
left a comment
There was a problem hiding this comment.
Heya! Nice!
Only thing I’d want is to move the caching part to a charactersToExpressionCached wrapper (which you can define right above charactersToExpression).
Otherwise, maybe a comment explaining why the weak map is so important (as you do here in this PR message).
Also, maybe good for reference, as this is 2 things in 1: do you have the separate numbers for a) weak map, b) hoisted regexes?
I’m 👍 to using |
|
I’ll work on the |
|
Updated!
For weak map only, I measured So it seems like hoisting the regex has only a small gain, but I think it's because stringify-entities/lib/core.js Lines 24 to 31 in 632a208 Skipping most of the regex work after it. Other usages for |
|
Thanks! Released in |
charactersToExpressionto prevent repeatedly created regexes dynamically.Benchmark
During my benchmarking of a project using
toHtmlheavily fromhast-util-to-html,toHtmltook about 6.06s in execution time. This PR drops it to 2.38s.toHtmluses this package which is hot code during itsserializeAttributecall. Examples: https://github.com/search?q=repo%3Asyntax-tree%2Fhast-util-to-html%20subset%3A&type=codeNotes on caching
The WeakMap caching for
charactersToExpressionmeans that if the array is mutated, the regex won't be re-built but I think this is a fine caveat that shouldn't be relied upon. If I remove the WeakMap caching, the benchmark only drops to 5.52s.In
hast-util-to-html, there are two places (lib/handle/text.jsandlib/handle/comment.js) that can yet to take advantage of the caching. If this PR is accepted, I can updatehast-util-to-htmlto do so, and I expect another 1s savings, bringing down to around 1.4s fortoHtml.