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

perf(SSR): remove slow regular expression for escapeHtml #12507

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ShuiRuTian
Copy link

@ShuiRuTian ShuiRuTian commented Dec 7, 2024

Regular expression is slow, removing them in escapeHtml should make things faster

The perf result shows it's 2x~3x faster: https://jsbench.me/rqm4ehfndc/1

Ping @edison1105 @Justineo @bgoscinski, for you commented on another perf PR, so I guess you might also have interest in this one :)

@Justineo
Copy link
Member

Justineo commented Dec 8, 2024

Creating an appropriate benchmark is quite complex. I attempted a slightly more complex one. During testing, 100 strings of lengths ranging from 3 to 100 are randomly generated, and they may or may not include characters that need escaping. As a comparison, I also added an implementation that uses regular expressions exclusively for the replacement: https://jsben.ch/uPxu5

In short, Vue's current implementation is consistently faster.

@ShuiRuTian
Copy link
Author

ShuiRuTian commented Dec 8, 2024

@Justineo Thanks a lot, however it seems you named the function mistakenly, oops

In https://jsben.ch/uPxu5, escapeHtmlHybrid contains the changes in this PR and escapeHtmlLoop is current implementation.

Then the result should be explained as "This PR is faster"

@Justineo
Copy link
Member

Justineo commented Dec 8, 2024

I’m AFK right now, but what you said sounds possible. I’ll take a look when I get back.

@Justineo
Copy link
Member

Justineo commented Dec 8, 2024

@ShuiRuTian Yes you are right. I've made an updated benchmark: https://jsben.ch/pbGau

Some additional findings:

  • The shorter the escaped string, the more noticeable the performance gap, which aligns with intuition—short strings can be fully processed with a single pass, so pre-checking for matches offers no real benefit.
  • In Firefox, the pure RegExp approach performs the best, with a significant advantage on longer strings.

@edison1105
Copy link
Member

LGTM.
Here is the bench result on Node v22.7.0, and indeed, the performance has improved with this PR.

 ✓ src/__benchmarks__/escapeHtml.bench.js (3) 1815ms
   ✓ escapeHtml (3) 1813ms
     name               hz     min     max    mean     p75     p99    p995    p999     rme  samples
   · PR #12507    5,661.77  0.1621  0.4674  0.1766  0.1761  0.2865  0.3176  0.3715  ±0.53%     2831   fastest
   · Vue current  5,195.69  0.1683  0.6889  0.1925  0.2057  0.3348  0.3768  0.5660  ±0.75%     2598
   · Pure RegExp  2,860.40  0.3260  2.0198  0.3496  0.3469  0.4891  0.6473  1.9624  ±1.29%     1431   slowest

 BENCH  Summary

  PR #12507 - src/__benchmarks__/escapeHtml.bench.js > escapeHtml
    1.09x faster than Vue current
    1.98x faster than Pure RegExp

@edison1105 edison1105 added ready to merge The PR is ready to be merged. 🧹 p1-chore Priority 1: this doesn't change code behavior. labels Dec 9, 2024
Copy link

github-actions bot commented Dec 9, 2024

Size Report

Bundles

File Size Gzip Brotli
runtime-dom.global.prod.js 100 kB 37.9 kB 34.2 kB
vue.global.prod.js 158 kB 57.8 kB 51.4 kB

Usages

Name Size Gzip Brotli
createApp (CAPI only) 47.2 kB (+510 B) 18.3 kB (+64 B) 16.8 kB (+60 B)
createApp 55.2 kB (+615 B) 21.3 kB (+74 B) 19.5 kB (+39 B)
createSSRApp 59.3 kB (+615 B) 23.1 kB (+77 B) 21 kB (+65 B)
defineCustomElement 60.1 kB (+645 B) 22.9 kB (+82 B) 20.8 kB (+66 B)
overall 69.1 kB (+705 B) 26.5 kB (+104 B) 24.1 kB (+68 B)

Copy link

pkg-pr-new bot commented Dec 9, 2024

Open in Stackblitz

@vue/compiler-core

npm i https://pkg.pr.new/@vue/compiler-core@12507

@vue/compiler-sfc

npm i https://pkg.pr.new/@vue/compiler-sfc@12507

@vue/compiler-ssr

npm i https://pkg.pr.new/@vue/compiler-ssr@12507

@vue/compiler-dom

npm i https://pkg.pr.new/@vue/compiler-dom@12507

@vue/runtime-core

npm i https://pkg.pr.new/@vue/runtime-core@12507

@vue/reactivity

npm i https://pkg.pr.new/@vue/reactivity@12507

@vue/runtime-dom

npm i https://pkg.pr.new/@vue/runtime-dom@12507

@vue/server-renderer

npm i https://pkg.pr.new/@vue/server-renderer@12507

@vue/shared

npm i https://pkg.pr.new/@vue/shared@12507

vue

npm i https://pkg.pr.new/vue@12507

@vue/compat

npm i https://pkg.pr.new/@vue/compat@12507

commit: e8e2fff

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🧹 p1-chore Priority 1: this doesn't change code behavior. ready to merge The PR is ready to be merged.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants