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

add skipStringOptimizations options to toString #58

Closed
wants to merge 3 commits into from

Conversation

Klaasvaak
Copy link

I'm optimizing the performance of a project I'm working on. It contains large paths (for example with a length of 160k). When skipping the string optimizations in toString I can see performance improvements around 7-11x. When having multiple of these large paths this adds up.

I made a simple example here: https://codepen.io/klaasvaak/pen/dyVjPje?editors=1011

Let me know if you have concerns about skipping these string optimizations.

@puzrin
Copy link
Member

puzrin commented Jan 6, 2022

Are you sure this option required for other users? For example, you could just override prototype method (or do it in inherited class)

@Klaasvaak
Copy link
Author

@puzrin Very good point. I will do this instead.

@Klaasvaak Klaasvaak closed this Jan 6, 2022
@puzrin
Copy link
Member

puzrin commented Jan 7, 2022

No problem, happy to help.

BTW

  • It's better to use benchmark.js, because single pass may be not enough to "warm up" JIT inline caches. See https://github.com/fontello/svgpath/tree/master/benchmark.
  • You could try to optimize existing .toString() instead. For example:
    • use String.test(regexp) before String.replace(regexp).
    • use more clever join, instead of join everything via space and then removing those back

      svgpath/lib/svgpath.js

      Lines 201 to 207 in 70134e4

      return elements.join(' ')
      // Optimizations: remove spaces around commands & before `-`
      //
      // We could also remove leading zeros for `0.5`-like values,
      // but their count is too small to spend time for.
      .replace(/ ?([achlmqrstvz]) ?/gi, '$1')
      .replace(/ \-/g, '-')
      . Existing code is from old time, when i tried to make all steps atomic, because was not certain about desired final sequence.

I did not experimented with this part of code too much. But after your example, i think there is room for improvements.

FYI. If you plan to dive into perfomance area, i strongly recommend to read this blog https://mrale.ph/. Because v8 JIT logic is completely different than anything "possible to imagine by normal human" :).

@Klaasvaak
Copy link
Author

@puzrin Thank you for sharing of this, this is very useful for me.

@puzrin
Copy link
Member

puzrin commented Jan 11, 2022

See v2.5.0.

./benchmark/benchmark.js 
.from("big.txt")

  > 2.4.1 x 10.33 ops/sec ±2.97% (30 runs sampled)
  > current x 10.33 ops/sec ±3.71% (30 runs sampled)

.from("one_path.txt")

  > 2.4.1 x 7,588 ops/sec ±2.90% (92 runs sampled)
  > current x 7,719 ops/sec ±1.42% (94 runs sampled)

.from(long).toString()

  > 2.4.1 x 3.17 ops/sec ±1.60% (12 runs sampled)
  > current x 103 ops/sec ±0.91% (74 runs sampled) <============

Enjoy!

@Klaasvaak
Copy link
Author

@puzrin This is great, thanks! I can confirm using the newest version .toString() in my project speeds things up as well :)

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

2 participants