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

Merged
merged 1 commit into from Apr 24, 2016

Conversation

Projects
None yet
2 participants
@flagbug
Contributor

flagbug commented Apr 23, 2016

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.

Don't compile the comment regex, because we only use every instance a…
… single time

This speeds up the comment formatting a gazillion times

@codecadwallader codecadwallader self-assigned this Apr 23, 2016

@codecadwallader

This comment has been minimized.

Owner

codecadwallader commented Apr 24, 2016

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

@codecadwallader codecadwallader added this to the v10.2 milestone Apr 24, 2016

@flagbug

This comment has been minimized.

Contributor

flagbug commented Apr 24, 2016

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!

@flagbug

This comment has been minimized.

Contributor

flagbug commented Apr 24, 2016

If I turn off reorganization and run just the cleanup step, it's pretty fast, only with the reorganization turned on it's slow 😅

@flagbug

This comment has been minimized.

Contributor

flagbug commented Apr 24, 2016

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!

@codecadwallader

This comment has been minimized.

Owner

codecadwallader commented Apr 24, 2016

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.

@codecadwallader codecadwallader merged commit 58205bb into codecadwallader:master Apr 24, 2016

1 check passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@codecadwallader

This comment has been minimized.

Owner

codecadwallader commented Apr 24, 2016

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/

@flagbug flagbug deleted the flagbug:reorganize-performance branch Apr 24, 2016

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