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

Optimize max_line_len #1109

Merged
merged 1 commit into from Nov 14, 2021
Merged

Conversation

jridgewell
Copy link
Collaborator

The old max_line_len code fell into a surprising slowdown where repeatedly inserting chars in the middle of a string falls into O(n^2) behavior. Surprising because V8 already implements advanced data structures for string slicing and joining (SlicedString and ConsString). It seems like this should be easily handled by the two.

Anyways, we avoid this by slowing building a committed string (which is unlikely to be changed). Whenever we insert, we commit the left hand side, and can avoid slicing it in the future.

In testing a 5mb file (done by repeating a program several times), I dropped the simple parse+output case from 5min to 5sec.

Fixes #374.

The old max_line_len code fell into a surprising slowdown where repeatedly inserting chars in the middle of a string falls into O(n^2) behavior. Surprising because V8 already implements advanced data structures for string slicing and joining (`SlicedString` and `ConsString`). It seems like this should be easily handled by the two.

Anyways, we avoid this by slowing building a committed string (which is unlikely to be changed). Whenever we insert, we commit the left hand side, and can avoid slicing it in the future.

In testing a 5mb file (done by repeating a program several times), I dropped the simple parse+output case from 5min to 5sec.
@fabiosantoscode
Copy link
Collaborator

This is crazy! I love it :)

I noticed has_nlb calls toString() and I thought it would be inefficient but probably it's not due to internal optimizations.

@fabiosantoscode fabiosantoscode merged commit a966e74 into terser:master Nov 14, 2021
@jridgewell
Copy link
Collaborator Author

Whoops, looks like I went to the wrong issue. This fixes #926.

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