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

refactor: dont covert to charcode in iswhitespace #2039

Merged
merged 1 commit into from
Jun 16, 2024

Conversation

SethFalco
Copy link
Member

@SethFalco SethFalco commented Jun 16, 2024

While looking through path.js, I noticed we had a isWsp function, which wasn't immediately obvious was an abbreviation for isWhiteSpace. So I renamed it to isWhiteSpace so it's obvious what it's doing.

From there, since we're stepping through a string char by char before calling the function, it's redundant to call codePointAt when we can just compare the string immediately. This is both faster and more readable for maintainers.

Finally, I just apply a proposal from SonarLint to replace an else { if () } with else if ().

Benchmark

const Benchmark = require('benchmark');
const suite = new Benchmark.Suite;

const data = [
  '0', // false
  '1', // false
  '-', // false
  '.', // false
  ' ', // true
  '\t', // true
  '\n', // true
  '\r', // true
  ' ', // false (technically true, but we didn't accept it before so false)
]

const isWhiteSpaceV1 = (c) => {
  const codePoint = c.codePointAt(0);
  return (
    codePoint === 0x20 ||
    codePoint === 0x9 ||
    codePoint === 0xd ||
    codePoint === 0xa
  );
};

const isWhiteSpaceV2 = (c) => {
  return (
    c === ' ' ||
    c === '\t' ||
    c === '\r' ||
    c === '\n'
  );
};

const set = new Set([' ', '\t', '\r', '\n']);
const isWhiteSpaceV3 = (c) => {
  return set.has(c);
};

suite.add('v1', () => {
  data.map((d) => isWhiteSpaceV1(d));
})
.add('v2', () => {
  data.map((d) => isWhiteSpaceV2(d));
})
.add('v3', () => {
  data.map((d) => isWhiteSpaceV3(d));
})
.on('cycle', (event) => {
  console.log(String(event.target));
})
.on('complete', function() {
  console.log('Fastest is ' + this.filter('fastest').map('name'));
})
.run({ 'async': true });
v1 x 28,871,343 ops/sec ±0.98% (86 runs sampled)
v2 x 50,999,166 ops/sec ±1.33% (90 runs sampled)
v3 x 15,390,647 ops/sec ±0.76% (96 runs sampled)
Fastest is v2

The impact of this change on our regression tests is negligible and within margin of error, but the code is cleaner imo, so I'm running with it.

Previous runs from other pull requests:

This run: 16 minutes 43 seconds

@SethFalco SethFalco merged commit b8f8d1c into svg:main Jun 16, 2024
12 checks passed
@SethFalco SethFalco deleted the perf-is-whitespace branch June 16, 2024 12:02
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.

1 participant