refactor(convertPaths): clean up plugin

[SethFalco]

While reviewing #1889 I'd made some changes locally which I just wanted to push.

This has no (or negligible) impact on performance, it's just to keep things cleaner.

• Make JSDocs with a single line, a single line.
• Declare variables individually in some places.
• Define type any on prev. We were literally disabling type checking with magic comments on every usage anyway.
• Use string.includes instead of ~string.indexOf as there is no significant difference while the former is more semantic.
• Stop using apply methods, we can just use the spread operator.
• Use Math.sqrt instead of Math.hypot as Math.hypot has overhead from improved precision, which we don't need here.
• Replace the regex decimal number check with a math implementation. It's just more logical, plus the original implementation would've failed on large integers or precision numbers anyway, i.e. 5e-324 (float) and 1.7976931348623157e+308 (integer)
• Rename some variables introduced in feat(convertPathData): allow converting q to t in more cases #1889 for clarity.

[KTibow]

Use Math.sqrt instead of Math.hypot as Math.hypot has overhead from improved precision, which we don't need here.

Did the testing really show that? I would think a function that does it all in native code must be faster than going back and forth between JS and native code.

Edit: According to https://github.com/bmeurer/js-micro-benchmarks/blob/60a34c0dd29b77e6950555c2dd9687b1a0a7671e/bench-math-hypot.js hypot is slower. That's weird but I won't question it.

[SethFalco]

Did the testing really show that? I would think a function that does it all in native code must be faster than going back and forth between JS and native code.

Yes and no! There is a measurable difference in benchmarks, but in practice the difference isn't really significant and may be margin of error (< 1 second). I opted to run with it since I took the time to check it out anyway.

Here are some benchmarks I ran:

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

const data = [
  [3, 4],
  [10, 10],
  [134, 5235],
  [352523, 52352],
  [525.52352, 532625.5235235],
  [4245, 1352.2352],
];

suite
  .add('Math.sqrt', () => data.map(([dx, dy]) => Math.sqrt(dx ** 2 + dy ** 2)))
  .add('Math.hypot', () => data.map(([dx, dy]) => Math.hypot(dx, dy)))
  .add('**', () => data.map(([dx, dy]) => (dx ** 2 + dy ** 2) ** 0.5))
  .on('cycle', (event) => console.log(String(event.target)))
  .run({ 'async': true });

// Math.sqrt x 5,724,619 ops/sec ±1.87% (89 runs sampled)
// Math.hypot x 3,313,527 ops/sec ±0.74% (95 runs sampled)
// ** x 5,466,879 ops/sec ±0.96% (95 runs sampled)

When looking into why Math.hypot was running slower, I found the following:

Math.hypot also avoids overflow/underflow problems if the magnitude of your numbers is very large. The largest number you can represent in JS is Number.MAX_VALUE, which is around 10^308.

— https://developer.mozilla.org/docs/Web/JavaScript/Reference/Global_Objects/Math/hypot#description

In other words, Math.hypot does a lot more than what Math.pow and Math.sqrt does, but we don't work at the scale or precision to benefit from it.

[XhmikosR]

I'd still use Math.hypot unless it the performance impact is really affecting svgo. It's more explicit and descriptive like what you did with string.includes Otherwise we would still use bitwise operators too, but descriptive and cleaner code is preferable IMHO unless the performance impact shows.

BTW, I recall I had attempted to apply some fixes (similar to the ones you make) across the codebase a couple of years ago which I found with xo, but we never landed them.

[SethFalco]

Otherwise we would still use bitwise operators too

This is different, actually. Math.sqrt and Math.hypot have a real difference both in benchmarks and in theory, though I'll admit in SVGO it's negligible. The bitwise operators aren't actually faster because it's being used in a loosely resolved conditional.

I assumed the bitwise operator was used for performance, so I did actually check before replacing that with String.includes. While ~'cs'.indexOf(…) is faster than 'cs'.includes(…), the loose conditional is slower and has a larger impact.

A faster approach would be to use a Set or even check the values literally, though. I forgot we could just do === since we weren't doing that to begin with. ^-^'

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

const data = [
  'c',
  's',
  'a',
  'C',
  'S',
  'A'
];

const str = 'cs';
const set = new Set([
  'c',
  's'
]);

suite
  .add('Bitwise indexOf', () => data.map((d) => ~str.indexOf(d) ? 1 : -1))
  .add('String includes', () => data.map((d) => str.includes(d) ? 1 : -1))
  .add('Set has', () => data.map((d) => set.has(d) ? 1 : -1))
  .add('Eq', () => data.map((d) => d === 'c' || d === 's' ? 1 : -1))
  .on('cycle', (event) => console.log(String(event.target)))
  .run({ 'async': true });

// Bitwise indexOf x 18,353,976 ops/sec ±2.19% (91 runs sampled)
// String includes x 18,999,212 ops/sec ±1.33% (94 runs sampled)
// Set has x 24,346,799 ops/sec ±1.79% (96 runs sampled)
// Eq x 59,498,845 ops/sec ±1.91% (93 runs sampled)

but descriptive and cleaner code is preferable IMHO unless the performance impact shows

I agree with this in general. The reason I opted to change it was just because I felt both were clean enough tbh.

However, given the difference is negligible, when I'm done with what I'm working on, I'll just double-check if it made a practical difference in our regression tests, and if not, I'll change it back. 👍🏽

Btw, the reason I'm nitpicking on performance so much is for this. I'm usually using a profiler, but the ones here were just me combing through while refactoring:

• test: expand and optimize regression testing SethFalco/svgo#1

I'd like to get this down as low as possible, ideally below 10 minutes, then merge it into the repo.

[XhmikosR]

Don't get me wrong, I'm all for performance myself. I just don't think it's worth reducing readability/maintainability unless we hit a critical path and performance is noticeable. but I think we are all on the same boat here :)