-
Notifications
You must be signed in to change notification settings - Fork 364
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
Speed up reorganization by a gazillion times #284
Speed up reorganization by a gazillion times #284
Conversation
… single time This speeds up the comment formatting a gazillion times
Thanks for identifying this optimization. It looks logical and matches recommendations found on MSDN. :) I see this optimization applying to speeding up the performance of comment formatting.. but I'm not sure how this code would ever be invoked from reorganization? The closest I could get was if you had reorganization and comment formatting enabled during cleanup then this code would be invoked.. but it isn't really related to reorganization at that point. Thoughts? Going deeper down the caching path seems like it could be worthwhile. There's actually a fair amount of caching natively available but it requires using the static Regex overloads which would require some rewrite. It would be fairly easy to create our own caches as well. https://msdn.microsoft.com/en-us/library/8zbs0h2f(v=vs.110).aspx |
Honestly, I'm not super sure if it's actually related to reorganization, I just noticed the reorganization step taking forever, so I attached the profiler and this stuck out like a sore thumb, so I fixed it! |
If I turn off reorganization and run just the cleanup step, it's pretty fast, only with the reorganization turned on it's slow 😅 |
I just looked at the profiler report agein, seem like you're right, the comment formatting is run in the cleanup code, seems like I got something mixed up! |
Well it's still certainly a performance improvement so very glad to have it. :) I just wanted to make sure I wasn't mis-understanding something before accepting it (which I'll do so now). If you see any other big opportunities for performance improvements they would be extremely welcome. I've used profilers before but it's been a little while and a review certainly wouldn't hurt. |
This change will be available in our CI channel momentarily here (v10.1.95 or higher): http://vsixgallery.com/extension/4c82e17d-927e-42d2-8460-b473ac7df316/ |
Previously, the regex to detect comments had the "Compile" flag enabled. Since each instance of the regex was only used once, this compiling could happen hundreds of times with a sufficiently large code file, which was super-bad for performance.
When the compile flag is removed, the reorganization time drops from potentially 10s of seconds to 1-2 seconds.
What we could do later, is compile the regex again, but lazily cache it somewhere, so only a single instance is ever reused, which could make the reorganization process even faster.