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

paths.normalize(): perf regression #7086

Closed
bpasero opened this issue Jun 1, 2016 · 9 comments
Closed

paths.normalize(): perf regression #7086

bpasero opened this issue Jun 1, 2016 · 9 comments
Assignees
Labels
bug Issue identified by VS Code Team member as probable bug important Issue identified as high-priority verified Verification succeeded
Milestone

Comments

@bpasero
Copy link
Member

bpasero commented Jun 1, 2016

Running the following test with profiling enabled shows that recent changes to paths.ts cause a major performance regression:

  • open the monaco folder
  • expand some folders
  • click refresh 10 times

Before:
image

After:
image

profiles.zip

@bpasero bpasero added bug Issue identified by VS Code Team member as probable bug important Issue identified as high-priority labels Jun 1, 2016
@jrieken jrieken added this to the June 2016 milestone Jun 1, 2016
@jrieken
Copy link
Member

jrieken commented Jun 1, 2016

impossible ;-)

@jrieken
Copy link
Member

jrieken commented Jun 1, 2016

Seems like it gets into the deopt-state cos it was optimised to many times...

@jrieken jrieken closed this as completed in 61fc69b Jun 1, 2016
@bpasero
Copy link
Member Author

bpasero commented Jun 2, 2016

Sorry I have to reopen this because I notice that the UI is freezing when I do a gulp watch from the command line while having Code open. I profiled the explorer refresh again as well as starting a profile before running gulp watch and measuring after this is done (vscode sourcecode workspace):

Explorer refresh new
image

Explorer refresh old
image

Compile all new
image

Compile all old
image

profiles.zip

@bpasero bpasero reopened this Jun 2, 2016
@jrieken
Copy link
Member

jrieken commented Jun 2, 2016

@bpasero I don't understand how that is related to gulp watch? This is outside of VS Code and triggered by file events?

@jrieken
Copy link
Member

jrieken commented Jun 2, 2016

might be an optimisation I removed to do nothing in case of an already normal string...

@bpasero
Copy link
Member Author

bpasero commented Jun 2, 2016

@jrieken the gulp watch causes file events to be triggered in relatively short time frame that probably spam our listeners in the window that all do normalization.

I think spamming file events is a sensitive topic for the workbench because many listeners are in the UI thread and most of them do path/uri magic. We could try to pull out more of that to the file watcher process but nevertheless it is a good test case for measuring performance in path/uri land.

@jrieken
Copy link
Member

jrieken commented Jun 3, 2016

@bpasero Please check this again if you don't mind. I took the shortcut to figure out first if a path is normal - which in case of gulp watch all paths are. The implementation doesn't use a regex anymore, is more correct than before, esp wrt double slashes. The sad part is that using charCodeAt isn't much faster than a regex...

@jrieken
Copy link
Member

jrieken commented Jun 3, 2016

Ok - hold on. RegEx for the win! Using /\/\.\.?\/|\/\.\.?$|^\.\.?\/|\/\/+|\\/ for checking posix paths is magnitudes after (23ms vs 120ms)

jrieken added a commit that referenced this issue Jun 3, 2016
@jrieken jrieken closed this as completed Jun 3, 2016
@bpasero bpasero assigned bpasero and unassigned jrieken Jun 3, 2016
@bpasero bpasero added the verified Verification succeeded label Jun 3, 2016
@bpasero
Copy link
Member Author

bpasero commented Jun 3, 2016

@jrieken yeap, back to good, thanks!

aeschli pushed a commit that referenced this issue Jun 6, 2016
@vscodebot vscodebot bot locked and limited conversation to collaborators Nov 18, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Issue identified by VS Code Team member as probable bug important Issue identified as high-priority verified Verification succeeded
Projects
None yet
Development

No branches or pull requests

2 participants