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

Scanner.js start-up performance could be improved #316

Closed
justjake opened this issue Feb 26, 2021 · 5 comments · Fixed by #318
Closed

Scanner.js start-up performance could be improved #316

justjake opened this issue Feb 26, 2021 · 5 comments · Fixed by #318
Assignees
Milestone

Comments

@justjake
Copy link

Hello y'all! I ran some profiles of Notion's startup performance today and noticed that Linkifyjs is very expensive to import. Specifically, the file linkifyjs/lib/linkify-core/scanner.js file appears to be quite expensive. I wonder if the tactic of using "a|b|c|...".split('|') is expensive, or if the creation of the parser states involves some kind of n^2 computation.

Anyways, I was surprised to find that Linkify was the single most expensive dependency that Notion imports, and I wanted to let you know in case you find the information useful!

Here's a screenshot from Chrome DevTools of the boot up of the Notion web-app in a production-like environment. You can see how importing linkify compares to some other 3rd party dependencies we use like Moment and Prism.js.

image

My suggestion is that you take some time to profile the scanner, and see what work you can defer to the first run of a public method, so the library is lazy internally.

@nfrasser
Copy link
Owner

Hi @justjake, thanks for reporting this, I'll be looking at this over the next couple of days

@nfrasser
Copy link
Owner

Narrowed down the performance hit to the construction of the state machine for handling the 1500+ TLDs: https://github.com/Soapbox/linkifyjs/blob/a565b6fa8ad095f6f3e453abe3fdfa7221040653/tlds.js

Emptying out the list narrows down the initialization time from ~32ms to ~2ms on my machine (converting from .split() to an array did not significantly improve performance)

Going to explore a few options to address this:

  • Precompute the state machine at build time (with a likely significant increase in file size)
  • Take out most of the TLDs and leave out only the most common ones like .com, .net, etc. with an optional "extended TLDs" plugin (breaking change)
  • Some other to-be-determined computational trick to speed up that code

@nfrasser
Copy link
Owner

  • Delay the initialization until Linkify is called for the first time

@nfrasser nfrasser self-assigned this Mar 4, 2021
@nfrasser
Copy link
Owner

nfrasser commented Mar 8, 2021

Good news, managed a combination of all of the above to get the initial load time down from ~30ms to ~1ms. Managed this with both revamped internals + delayed initialization. The means there's an extra penalty you now pay the first time you run linkify (only about ~4ms thanks to the revamped internals), but future runs are faster than ever.

The fix is now available in v3.0.0-beta.1. A production release will be out in the coming weeks.

v2 vs v3 performance comparison

@nfrasser nfrasser mentioned this issue Mar 8, 2021
@nfrasser nfrasser added this to the 3.0 milestone Mar 8, 2021
@justjake
Copy link
Author

Amazing!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants