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

Jump to bottom

Faster SSR #5701

Merged
merged 11 commits into from May 13, 2022
Merged

Faster SSR #5701

Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
aa15e9a
faster SSR, modelled on ryansolid/dom-expressions#27
Rich-Harris Nov 20, 2020
e58d658
simplify
Rich-Harris Nov 20, 2020
57bb045
remove redundant parens
Rich-Harris Nov 20, 2020
29d4695
boolean argument
Rich-Harris Nov 20, 2020
3a94f2a
Update src/runtime/internal/ssr.ts
Rich-Harris Nov 20, 2020
bc4bda9
merge latest changes from master
benmccann May 6, 2022
30df740
fix merge
benmccann May 6, 2022
88e2146
use ternary
benmccann May 6, 2022
1b0e836
slice -> substring
benmccann May 6, 2022
61e180b
add comment
benmccann May 6, 2022
b32049b
restore String conversion
benmccann May 6, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/compiler/compile/render_ssr/handlers/shared/get_attribute_value.ts
View file
Open in desktop
Expand Up @@ -9,7 +9,7 @@ export function get_class_attribute_value(attribute: Attribute): ESTreeExpressio
// handle special case — `class={possiblyUndefined}` with scoped CSS
if (attribute.chunks.length === 2 && (attribute.chunks[1] as Text).synthetic) {
const value = (attribute.chunks[0] as Expression).node;
return x`@escape(@null_to_empty(${value})) + "${(attribute.chunks[1] as Text).data}"`;
return x`@escape(@null_to_empty(${value}), true) + "${(attribute.chunks[1] as Text).data}"`;
}

return get_attribute_value(attribute);
Expand All @@ -22,7 +22,7 @@ export function get_attribute_value(attribute: Attribute): ESTreeExpression {
.map((chunk) => {
return chunk.type === 'Text'
? string_literal(chunk.data.replace(/"/g, '"')) as ESTreeExpression
: x`@escape(${chunk.node})`;
: x`@escape(${chunk.node}, true)`;
})
.reduce((lhs, rhs) => x`${lhs} + ${rhs}`);
}
29 changes: 22 additions & 7 deletions src/runtime/internal/ssr.ts
View file
Open in desktop
Expand Up @@ -32,16 +32,31 @@ export function spread(args, classes_to_add) {
return str;
}

export const escaped = {
const ATTR_REGEX = /[&"]/g;
benmccann marked this conversation as resolved.
Show resolved Hide resolved
const CONTENT_REGEX = /[&<]/g;

const escapes = {
'"': '&quot;',
"'": '&#39;',
'&': '&amp;',
'<': '&lt;',
'>': '&gt;'
'<': '&lt'
benmccann marked this conversation as resolved.
Show resolved Hide resolved
};

export function escape(html) {
return String(html).replace(/["'&<>]/g, match => escaped[match]);
export function escape(html: string, is_attr = false) {
benmccann marked this conversation as resolved.
Show resolved Hide resolved
if (typeof html !== 'string') return html;
benmccann marked this conversation as resolved.
Show resolved Hide resolved

const pattern = is_attr ? ATTR_REGEX : CONTENT_REGEX;
pattern.lastIndex = 0;

let escaped = '';
let last = 0;

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

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

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

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 •
edited

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

Copy link

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.

Copy link

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...

last = i + 1;
}

return escaped + html.slice(last);
}

export function each(items, fn) {
Expand Down Expand Up @@ -129,7 +144,7 @@ export function create_ssr_component(fn) {

export function add_attribute(name, value, boolean) {
if (value == null || (boolean && !value)) return '';
return ` ${name}${value === true ? '' : `=${typeof value === 'string' ? JSON.stringify(escape(value)) : `"${value}"`}`}`;
return ` ${name}${value === true ? '' : `=${typeof value === 'string' ? JSON.stringify(escape(value, true)) : `"${value}"`}`}`;
}

export function add_classes(classes) {
Expand Down