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

Long text in JSX is slow #3263

Closed
k15a opened this issue Nov 14, 2017 · 5 comments
Closed

Long text in JSX is slow #3263

k15a opened this issue Nov 14, 2017 · 5 comments
Labels
lang:jsx Issues affecting JSX (not general JS issues) locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting.
Milestone

Comments

@k15a
Copy link
Collaborator

k15a commented Nov 14, 2017

If you have a lot of text in JSX Prettier is really slow. It takes around 2 seconds to print a file with 1000 lines of text where it takes only 200 ms to print a file with 1000 lines of tags.

@azz azz added the lang:jsx Issues affecting JSX (not general JS issues) label Nov 14, 2017
@azz
Copy link
Member

azz commented Nov 14, 2017

We should profile it, see what's eating up the CPU.

@duailibe
Copy link
Member

duailibe commented Nov 14, 2017

I did a very quick test, with a large file that took prettier ~3.6s to format, and it looks the first culprit is a call to assertDoc in fill():

function fill(parts) {
parts.forEach(assertDoc);
return { type: "fill", parts };
}

Which is a very simple code (some typeof checks that shouldn't be too slow), but it's called N times in printDocToString:

prettier/src/doc-printer.js

Lines 300 to 301 in 2332c5c

const remaining = parts.slice(2);
const remainingCmd = [ind, mode, fill(remaining)];

Making that algorithm quadratic if I did understood correctly. Removing the check completely reduced the time to ~1.1s.

With that out of that way, the 2nd culprit is slicing that very large array in printDocToString (see code snippet above) for every pair of elements (also making it quadratic). Changing the code to mutate the array, reduced the time to ~250ms.

Then I re-added the assertDoc calls and the time went up to ~2.8s.

@duailibe
Copy link
Member

duailibe commented Nov 16, 2017

I think we're good now, reduced to about ~20% of the time :-)

@karl
Copy link
Collaborator

karl commented Nov 16, 2017

Awesome work on this @duailibe!

That is a huge improvement over my naive implementation of fill.

@duailibe
Copy link
Member

Thanks! Thankfully the code is really well written it was easy to understand it 😄

@suchipi suchipi added this to the 1.9 milestone Nov 28, 2017
@lock lock bot added the locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting. label Jul 6, 2018
@lock lock bot locked as resolved and limited conversation to collaborators Jul 6, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lang:jsx Issues affecting JSX (not general JS issues) locked-due-to-inactivity Please open a new issue and fill out the template instead of commenting.
Projects
None yet
Development

No branches or pull requests

5 participants