-
-
Notifications
You must be signed in to change notification settings - Fork 4.5k
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
feat: --debug
prints time it takes to parse a file
#15609
Conversation
Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
--debug
prints time it takes to parse a file
lib/linter/linter.js
Outdated
const scopeManager = parseResult.scopeManager || analyzeScope(ast, languageOptions, visitorKeys); | ||
|
||
debug("Parsing successful:", filePath); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Espree (default parser) doesn't provide parseResult.scopeManager
, so this would include time spent in a separate scope analysis (by eslint-scope), which might be a significant % of the total time between these two debug logs.
I don't know if this makes more sense for your analysis, just want to check if the intent is to calculate parsing time + scope analysis time, or it might be better to split them with more debug logs?
For context, some parsers, like @typescript-eslint/parser
, provide their own scope analysis (as parseResult.scopeManager
) so in that case there would be no difference since both parsing and scope analysis are done in parser.parseForESLint()
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good point, I think it might be useful to have time taken for both steps logged separately. Could you hint how this could be solved generically that would handle built-in parser as well as @typescript-eslint/parser
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
how this could be solved generically that would handle built-in parser as well as
@typescript-eslint/parser
We can add one pair of debug logs around const parseResult = ...
, and another pair around const scopeManager = ...
.
When the built-in parser is used, that would log parse time and scope time separately.
When @typescript-eslint/parser
is used, parse time and scope time would be both included in the first log (the second would be ~0 time) and it doesn't seem there's anything we can do to split them here. The only solution might be to ask @typescript-eslint/parser
to add logs around this line. Then, it would be possible to see parse + scope
time (logged by ESLint) and scope
time (logged by @typescript-eslint/parser
) ,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only solution might be to ask
@typescript-eslint/parser
to add logs around this line. Then, it would be possible to seeparse + scope
time (logged by ESLint) andscope
time (logged by@typescript-eslint/parser
) ,
In this case, since both packages are using the same debug
library, you could set environment variable DEBUG=eslint:linter,typescript-eslint:parser:parser
and run eslint without --debug
to see logs from both modules.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @mdjermanovic, I applied your suggestions
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [eslint](https://eslint.org) ([source](https://github.com/eslint/eslint)) | devDependencies | minor | [`8.9.0` -> `8.10.0`](https://renovatebot.com/diffs/npm/eslint/8.9.0/8.10.0) | --- ### Release Notes <details> <summary>eslint/eslint</summary> ### [`v8.10.0`](https://github.com/eslint/eslint/releases/v8.10.0) [Compare Source](eslint/eslint@v8.9.0...v8.10.0) #### Features - [`6e2c325`](eslint/eslint@6e2c325) feat: Add `ignoreOnInitialization` option to no-shadow rule ([#​14963](eslint/eslint#14963)) (Soufiane Boutahlil) - [`115cae5`](eslint/eslint@115cae5) feat: `--debug` prints time it takes to parse a file ([#​15609](eslint/eslint#15609)) (Bartek Iwańczuk) - [`345e70d`](eslint/eslint@345e70d) feat: Add `onlyOneSimpleParam` option to no-confusing-arrow rule ([#​15566](eslint/eslint#15566)) (Gautam Arora) #### Bug Fixes - [`cdc5802`](eslint/eslint@cdc5802) fix: Avoid `__dirname` for built-in configs ([#​15616](eslint/eslint#15616)) (DoZerg) - [`ee7c5d1`](eslint/eslint@ee7c5d1) fix: false positive in `camelcase` with combined properties ([#​15581](eslint/eslint#15581)) (Nitin Kumar) #### Documentation - [`1005bd5`](eslint/eslint@1005bd5) docs: update CLA information ([#​15630](eslint/eslint#15630)) (Nitin Kumar) - [`5d65c3b`](eslint/eslint@5d65c3b) docs: Fix typo in `no-irregular-whitespace` ([#​15634](eslint/eslint#15634)) (Ryota Sekiya) - [`b93af98`](eslint/eslint@b93af98) docs: add links between rules about whitespace around block curly braces ([#​15625](eslint/eslint#15625)) (Milos Djermanovic) - [`ebc0460`](eslint/eslint@ebc0460) docs: update babel links ([#​15624](eslint/eslint#15624)) (Milos Djermanovic) #### Chores - [`7cec74e`](eslint/eslint@7cec74e) chore: upgrade [@​eslint/eslintrc](https://github.com/eslint/eslintrc)[@​1](https://github.com/1).2.0 ([#​15648](eslint/eslint#15648)) (Milos Djermanovic) - [`11c8580`](eslint/eslint@11c8580) chore: read `ESLINT_MOCHA_TIMEOUT` env var in Makefile.js ([#​15626](eslint/eslint#15626)) (Piggy) - [`bfaa548`](eslint/eslint@bfaa548) test: add integration tests with built-in configs ([#​15612](eslint/eslint#15612)) (Milos Djermanovic) - [`39a2fb3`](eslint/eslint@39a2fb3) perf: fix lazy loading of core rules ([#​15606](eslint/eslint#15606)) (Milos Djermanovic) - [`3fc9196`](eslint/eslint@3fc9196) chore: include `tests/conf` in test runs ([#​15610](eslint/eslint#15610)) (Milos Djermanovic) </details> --- ### Configuration 📅 **Schedule**: At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, click this checkbox. --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate). Co-authored-by: cabr2-bot <cabr2.help@gmail.com> Reviewed-on: https://codeberg.org/Calciumdibromid/CaBr2/pulls/1190 Reviewed-by: 6543 <6543@noreply.codeberg.org> Co-authored-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org> Co-committed-by: Calciumdibromid Bot <cabr2_bot@noreply.codeberg.org>
* feat: --debug prints time it takes to parse a file * Update lib/linter/linter.js Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com> * add separate logs for parsing and scope analysis * lint Co-authored-by: Milos Djermanovic <milos.djermanovic@gmail.com>
Prerequisites checklist
What is the purpose of this pull request? (put an "X" next to an item)
[ ] Documentation update
[ ] Bug fix (template)
[ ] New rule (template)
[ ] Changes an existing rule (template)
[ ] Add autofix to a rule
[ ] Add a CLI option
[x] Add something to the core
[ ] Other, please explain:
What changes did you make? (Give an overview)
Is there anything you'd like reviewers to focus on?
Closes #15585
I think this is enough debug output, if parsing takes less <1ms then its cost is negligible anyway.