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

Add grammar performance warning #128

Merged
merged 4 commits into from
Apr 27, 2020
Merged

Conversation

Yanpas
Copy link
Contributor

@Yanpas Yanpas commented Mar 16, 2020

@Yanpas
Copy link
Contributor Author

Yanpas commented Mar 16, 2020

This code is executed in the browser context, so there is no need in perf_hooks module

@alexdima alexdima self-requested a review March 16, 2020 16:03
Copy link
Member

@alexdima alexdima left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Yanpas
Thank you for your PR. But:

  1. This node module needs to execute in browsers and in nodejs, so we cannot use perf_hooks.
  2. We cannot just console.warn with a somewhat random time limit. VS Code ships to millions of devices with different specifications, and such a performance problem should be investigated by the grammar author, and no cycles should be spent to record duration on end users machines. In other words, IMHO, it is not useful to run this troubleshooting code all the time.

That being said, perhaps the Registry or the tokenize call can get some kind of option called "profile: true" or something. Then, only if that option is set, the times will be recorded. Then, we can adopt this in VS Code and check if running from source and then enable that flag.

@Yanpas
Copy link
Contributor Author

Yanpas commented Mar 16, 2020

  1. I can use smth like (performance as any)?.now(). (Sadly this is not merged perf_hooks: expose performance global nodejs/node#28635)
  2. I think we can reuse DebugFlags.InDebugMode. It is enabled easily via VSCODE_TEXTMATE_DEBUG env.

@alexdima
Copy link
Member

  • DebugFlags.InDebugMode makes sense.
  • Date.now() can be used everywhere.

@Yanpas
Copy link
Contributor Author

Yanpas commented Mar 31, 2020

Date.now() is unsuitable cause it's not monotonic. (You called Date.now, OS scheduler stopped the process, some other process did it's work (language server e.g.), now our process has CPU again and consecutive call Date.now will return all time that has passed in real life, not process's life.

So I see no other option as proposed above

@Yanpas
Copy link
Contributor Author

Yanpas commented Apr 16, 2020

Updated branch

@alexdima alexdima added this to the April 2020 milestone Apr 27, 2020
@alexdima alexdima merged commit afb0dd5 into microsoft:master Apr 27, 2020
@alexdima
Copy link
Member

Thank you!

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.

2 participants