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

Faster SSR #5701

Merged
merged 11 commits into from
May 13, 2022
Merged

Faster SSR #5701

merged 11 commits into from
May 13, 2022

Conversation

Rich-Harris
Copy link
Member

Before submitting the PR, please make sure you do the following

  • It's really useful if your PR references an issue where it is discussed ahead of time. In many cases, features are absent for a reason. For large changes, please create an RFC: https://github.com/sveltejs/rfcs
  • This message body should clearly illustrate what problems it solves.
  • Ideally, include a test that fails without this PR but passes with it.

Inspired by ryansolid/dom-expressions#27, this replaces the current escape function, used in SSR to escape <, " and & characters, with a much faster version. It's not the same function in that PR, it's a smaller and (in my measurements) faster one that skips ahead to the next escapable character rather than incrementing one character at a time.

It has quite a remarkable impact on performance. Using the marko-js/isomorphic-ui-benchmarks repo, we see the following results:

3.29.7 this PR
search-results benchmark 2,496 ops/sec 4,748 ops/sec (1.90x faster)
color-picker benchmark 7,288 ops/sec 22,196 ops/sec (3.05x faster)

It no longer escapes > and ' characters because as far as I can tell that's not necessary. No tests needed to change.

Tests

  • Run the tests with npm test and lint the project with npm run lint

@Conduitry
Copy link
Member

The build's currently failing - it looks like because of a typescript type mismatch.

@pushkine
Copy link
Contributor

pushkine commented Nov 20, 2020

The argument is server side, but is there a reason to use numbers instead of booleans anywhere at all ?

I doubt the difference of one single easily gzip-able ascii character from !0 to 1 justifies the runtime cost of initializing a 32bit number, and evaluating truthiness is probably magnitudes faster using booleans

The cost is so tiny it shouldn't warrant an argument, yet it also impedes on readability in the codebase, hence my question


while (pattern.test(html)) {
const i = pattern.lastIndex - 1;
escaped += html.slice(last, i) + escapes[html[i]];
Copy link
Contributor

@ehrencrona ehrencrona Nov 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for what it's worth, replacing escapes[html[i]] with const ch = html[i] and then (ch === '&' ? ch === '"' ? '&quot;' : '&amp;' : '&lt;') seems to shave another 10% off the execution time when I test it.

but i'm surprised the regexp performs so badly; interestingly enough, it's the function as second parameter that's the slowdown. html.replace(/&/g, '&amp;').replace(/</g, '&lt;') is pretty much exactly the same speed as the new code.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow. Maybe we should just do that instead then? How are you getting those numbers, and can you share them?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just took a 500 kb HTML document and ran

const html = readFileSync('bigdocument.html').toString();

const start = performance.now();

for (let i = 0; i < 2000; i++) {
  escape(html);
}

console.log(performance.now() - start, 'ms');

Our documents of course tend to be smaller so there is a risk this doesn't correspond to real-life performance.

Copy link
Contributor

@ehrencrona ehrencrona Nov 20, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The .replace.replace version runs in 3.5s, the code in the PR with escapes[html[i]] in 3.6s, if you replace it by the ternary operator in about 3.2s, and the original code with a function as second parameter to replace in 14.5s.

Copy link

@joshgoebel joshgoebel Nov 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my testing a switch on character code (charCodeAt) is WAY faster than a simple object lookup. Don't ask me why. IE: (from another implementation)

  while (match = matchHtmlRegExp.exec(str)) {
    switch (str.charCodeAt(match.index)) {
      case 34: // "
        escape = '&quot;'
        break

I forget but it was something like 240 ops/sec vs 280 ops/sec (on a large file). I just submitted a 30% speed improvement patch to replace-html library.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(ch === '&' ? ch === '"' ? '"' : '&' : '<')

I believe this is broken and it needs to be (ch === '&' ? '&amp;' : (ch === '"' ? '&quot;' : '&lt;')). It does appear faster than the original version in this PR

Copy link
Member

@benmccann benmccann May 6, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The switch method is slower than both the ternary and the original PR implementation in my testing with @ehrencrona's benchmark above

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The switch method is slower than both the ternary and the original PR implementation in my testing

Another thing we have to keep in mind here is that almost 2 years have passed. It's very hard IMHO to micro-tune JS performance (over the long-term). Engines vary, browsers vary, things change with time. The thing that was fastest 2 years ago might bench worse today. The regex engines used in the major browsers are not all created equal.

We should be clear if we're talking benchmarks which browser, which engine... in the Svelt context perhaps we only care about Node? Just a consideration.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I tested on Node. This is for server-side code, so no need to test in browsers.

Thanks for sharing your findings! It may well have been different in the past like you said and regardless it was valuable to test various ideas and ensure we're doing as well as possible.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh for sure... we can certainly chase the peak performance (and probably should)... but we just may have to do so again in another 2 years - and make sure we're always comparing apples to apples. :). If we only need to test on Node that certainly helps a lot. Though it might not surprise me to learn if there were say differences between Node on ARM vs Node on x86_64...

Co-authored-by: Conduitry <git@chor.date>
@Rich-Harris
Copy link
Member Author

I doubt the difference of one single easily gzip-able ascii character from !0 to 1 justifies the runtime cost of initializing a 32bit number, and evaluating truthiness is probably magnitudes faster using booleans

According to an untrustworthy benchmark I just cobbled together...

iterations = 1e7;

function number() {
  console.time('number');
  let total = 0;
  let i = iterations;
  while (i--) {
    const n = i % 2 ? 0 : 1;
    if (n) {
      total += 1;
    }
  }
  console.timeEnd('number');

  return total;
}

function boolean() {
  console.time('boolean');
  let total = 0;
  let i = iterations;
  while (i--) {
    const b = i % 2 ? false : true;
    if (b) {
      total += 1;
    }
  }
  console.timeEnd('boolean');

  return total;
}

...the boolean is indeed reliably faster. TIL! My assumption had always been that we'd still be better off, since minifiers typically rewrite true and false as !0 and !1, meaning that you pay for the extra byte and for the coercion, but it turns out not to be the case:

function coerced_number() {
  console.time('coerced_number');
  let total = 0;
  let i = iterations;
  while (i--) {
    const n = i % 2 ? !1 : !0;
    if (n) {
      total += 1;
    }
  }
  console.timeEnd('coerced_number');

  return total;
}

For whatever reason, coerced_number takes about the same time as boolean, and both are reliably faster than number. Weird. So yeah, I guess we probably should use booleans everywhere.

@lukeed
Copy link
Member

lukeed commented Nov 20, 2020

Few minor things:

  • boolean coercion is expensive (!0 <<< true)

  • Original escape_chars dict should probably be Object.create(null) with keys added. Key-lookup are cheap but not free. Falls inline with @ehrencrona's findings

  • If this approach is to stay, then substring is typically faster than slicing

@benmccann benmccann added the perf label May 10, 2021
@stale
Copy link

stale bot commented Dec 7, 2021

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale-bot label Dec 7, 2021
Copy link
Member

@benmccann benmccann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've updated this PR and addressed all the comments. LGTM now

@mrkishi
Copy link
Member

mrkishi commented May 6, 2022

It seems Svelte doesn't escape invalid surrogate pairs — I wonder if this should go in this PR?

To be clear, it seems niche enough that it was never an issue, so the alternative could be to just not deal with it.

@benmccann
Copy link
Member

I'm not entirely sure I understand the issue, but it sounds outside the scope of this PR

src/runtime/internal/ssr.ts Outdated Show resolved Hide resolved
src/runtime/internal/ssr.ts Show resolved Hide resolved
src/runtime/internal/ssr.ts Outdated Show resolved Hide resolved
src/runtime/internal/ssr.ts Outdated Show resolved Hide resolved
@benmccann benmccann merged commit e7a2350 into master May 13, 2022
@stalkerg
Copy link
Contributor

Interesting, is it possible to use indexOf instead RegExp here? Let's see...

@intrnl
Copy link

intrnl commented Jul 24, 2022

This is a very late comment but wouldn't this be even faster if regular expressions are skipped entirely?

https://jsbench.me/g0l5yr4wge/2

EDIT:
I've done more benchmarking on this.
https://gist.github.com/intrnl/07816a7669d0ad4620390bd69e59b9d4

@bluwy
Copy link
Member

bluwy commented Jul 25, 2022

@intrnl Looks like escape_no_re works better for shorter strings, while escape_new is better for longer strings. I guess we'd want to cover cases for longer strings more so escape_new could still be good, though I'm not certain how this would play out in practice.

@stalkerg
Copy link
Contributor

@intrnl @bluwy I suppose it will be better to use indexOf instead iterate over the string.

@benmccann
Copy link
Member

benmccann commented Jul 27, 2022

Maybe the best is to choose a method based on string length? Though I imagine that could vary based on hardware

@intrnl
Copy link

intrnl commented Jul 27, 2022

@intrnl I suppose it will be better to use indexOf instead iterate over the string.

I'm not sure about the indexOf approach, as that would mean reiterating through the entire string twice (& and either < or "), will have to see.


I brought this up again mainly because I'm rather interested on seeing what people commonly interpolate, is it short? is it long? does it have a ton of instances of &<"? I do agree that this might be something that's not worth fussing a lot over though as this is too situational 😅

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.