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

Fast Remove #610

Open
dsherret opened this issue Apr 12, 2019 · 4 comments
Open

Fast Remove #610

dsherret opened this issue Apr 12, 2019 · 4 comments

Comments

@dsherret
Copy link
Owner

dsherret commented Apr 12, 2019

As part of the future performance improvements, working on improving the #remove() method's performance might be a good start.

On remove, the following happens today:

  1. Source file text is changed to not include the node's text.
  2. Source file text is reparsed.
  3. Wrapped nodes are updated with new compiler nodes.

A better solution, but a bit more complicated would be:

  1. Source file text is changed to not include the node's text.
  2. Existing AST is manipulated to remove the removed node (ex. update #statements and #_children when removing a statement)
  3. Go up the parents, and down the following nodes to change any pos and end properties. Also, remove stuff like symbols and such (need to investigate more)
  4. Update the source file text.
  5. Forget the removed node.

Not having to reparse and fill the wrapped nodes with new nodes should improve the performance big time. This change will have no effect on the public API.

Note: The tests for these should do a deep search of the final AST to ensure the previous compiler node isn't remaining inside the AST somewhere and that no symbols exist hidden within the AST. I'll need to edit private and internal data in the nodes to do this.

When adding this, be sure to add some metrics to the project specifically for this scenario (need line graph showing performance over every commit... do worst case and best case on some generated data).

@dsherret
Copy link
Owner Author

dsherret commented Apr 13, 2019

I hacked together something for this today and this was 10x faster than the current way on the 002_Removing.ts performance test. So this is a must do, but will be a bit challenging.

@dsherret
Copy link
Owner Author

I was talking with Titian at tsconf and it seems I could maybe take advantage of the incremental parser here. The function of interest seems to be updateLanguageServiceSourceFile.

@bombillazo
Copy link

Hey, we have an autogenerate file with over 200,000 lines of code, and removing nodes from this file has become impossible due to heap limit issues... :/ I tried forgetting nodes, batching node removal, etc but nothing works. Im thinking will have to resort to straight up string manipulation :/

@bombillazo
Copy link

For more context, it doesnt outright not work, i iterate various nodes (both while analyzing and already filtered in an array) and at around 40 nodes removes, the heap limit is reached. I also tried reassigning/re-reading the file and project to the variable, hoping the JS garbage collector would clear some space, but it didn't work.

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

No branches or pull requests

2 participants