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(escapeHTML): use multiple replace #178

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

SukkaW
Copy link
Member

@SukkaW SukkaW commented Jan 28, 2020

Benchmark: https://runkit.com/sukkaw/5e3003ffe7e84c0013f6210d

const Benchmark = require('benchmark');
const Suite = new Benchmark.Suite;
const html = `<p class="foo">Hello "world".</p><p class="foo">Hello "world".</p><p class="foo">Hello "world".</p><p class="foo">Hello "world".</p><p class="foo">Hello "world".</p><p class="foo">Hello "world".</p><p class="foo">Hello "world".</p><p class="foo">Hello "world".</p><p class="foo">Hello "world".</p><p class="foo">Hello "world".</p><p class="foo">Hello "world".</p><p class="foo">Hello "world".</p><p class="foo">Hello "world".</p><p class="foo">Hello "world".</p><p class="foo">Hello "world".</p><p class="foo">Hello "world".</p><p class="foo">Hello "world".</p><p class="foo">Hello "world".</p><p class="foo">Hello "world".</p><p class="foo">Hello "world".</p><p class="foo">Hello "world".</p><p class="foo">Hello "world".</p><p class="foo">Hello "world".</p><p class="foo">Hello "world".</p><p class="foo">Hello "world".</p><p class="foo">Hello "world".</p><p class="foo">Hello "world".</p>`;
const htmlEntityMap = {
  '&': '&amp;',
  '<': '&lt;',
  '>': '&gt;',
  '"': '&quot;',
  '\'': '&#39;',
  '`': '&#96;',
  '/': '&#x2F;',
  '=': '&#x3D;'
};

Suite.add('Map replacement', () => {
  html.replace(/[&<>"'`/=]/g, a => htmlEntityMap[a]);
}).add('Multiple replace', () => {
  html.replace(/&/g, '&amp;').replace(/</g, '&lt;').replace(/>/g, '&gt;').replace(/"/g, '&quot;').replace(/\\/g, '&#39;').replace(/`/g, '&#96;').replace(/\//g, '&#x2F').replace(/=/g, '&#x3D');
}).on('cycle', function(event) {
  console.info(String(event.target));
}).run();

On my local computer, Multiple replace is the slower one under Node.js 10 but it is the faster one under Node.js 12 & 13:

image

@SukkaW SukkaW requested a review from a team January 28, 2020 10:24
@coveralls
Copy link

coveralls commented Jan 28, 2020

Coverage Status

Coverage decreased (-0.01%) to 96.979% when pulling 087af71 on SukkaW:perf-escape-unescape into 4e7c24e on hexojs:master.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.01%) to 96.979% when pulling 087af71 on SukkaW:perf-escape-unescape into 4e7c24e on hexojs:master.

@seaoak
Copy link
Member

seaoak commented Jan 30, 2020

I modified the source text to include all special characters.

const html = `<p class="foo">Hello "world&".</p><p class="foo">Hello "world\"".</p><p class="foo">Hello "world\`".</p><p class="foo">Hello "world=".</p><p class="foo">Hello "world\\".</p><p class="foo">Hello "world&".</p><p class="foo">Hello "world\"".</p><p class="foo">Hello "world\`".</p><p class="foo">Hello "world=".</p><p class="foo">Hello "world\\".</p><p class="foo">Hello "world&".</p><p class="foo">Hello "world\"".</p><p class="foo">Hello "world\`".</p><p class="foo">Hello "world=".</p><p class="foo">Hello "world\\".</p><p class="foo">Hello "world&".</p><p class="foo">Hello "world\"".</p><p class="foo">Hello "world\`".</p><p class="foo">Hello "world=".</p><p class="foo">Hello "world\\".</p><p class="foo">Hello "world&".</p><p class="foo">Hello "world\"".</p><p class="foo">Hello "world\`".</p><p class="foo">Hello "world=".</p><p class="foo">Hello "world\\".</p><p class="foo">Hello "world".</p><p class="foo">Hello "world".</p>`;

The result of benchmark with Node v12 is reversal.
Multiple replace is slower.

SS20200130b0 perf_escapeHTML_v12

The result of benchmark with Node v13 is same as SukkaW's report.
Multiple replace is faster.

SS20200130a0 perf_escapeHTML_v13

It is certain that my sample source text is unusual HTML.
I only show that this patch is not all-around.
Because this patch sacrifices readability,
the profit seems too little.

@SukkaW
Copy link
Member Author

SukkaW commented Jan 16, 2022

More performance benchmark needs to be done before we merge this.

@SukkaW SukkaW marked this pull request as draft January 16, 2022 04:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants