Skip parsing JSDoc in .ts files #52466
Closed
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Just an experiment for now. Current test failures: - all fourslash tests of jsdoc fail; the parser should produce jsdoc for tsserver. - Spurious unused-type errors because `@link` isn't parsed and treated as a usage. - ohhh no, `@this` is mistakenly interpreted in .ts files: thisInFunctionCall.
typescript-bot
added
Author: Team
For Uncommitted Bug
|
@typescript-bot perf test this |
|
Heya @sandersn, I've started to run the perf test suite on this PR at 5d585f6. You can monitor the build here. Update: The results are in! |
|
@sandersn Here they are:
CompilerComparison Report - main..52466
System
Hosts
Scenarios
TSServerComparison Report - main..52466
System
Hosts
Scenarios
StartupComparison Report - main..52466
System
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Holy moly, we spend that much time on JSDoc? |
|
Yes |
|
Yep. Yes indeed. |
also delete comment in checker, that's for a different experiment
|
@typescript-bot perf test this |
|
Heya @sandersn, I've started to run the perf test suite on this PR at 3d5d2a6. You can monitor the build here. Update: The results are in! |
|
This looks great, hah. Do the parser time savings in our benches correspond, percentage-wise, with how much text is jsdoc, or does jsdoc have an outsized parse time impact? I ask because, if the later, it may still be worth also looking at if we can make jsdoc parsing faster for the sake of LS operations that actually need the jsdoc. |
Uhhh, not really? Correlation is 27%, which is not high, although xstate is kind of an outlier and if you drop it the correlation is 66%. It's definitely worth investigating jsdoc parsing. Edit: There are only 50,000 characters of comments in xstate so I looked at all of them. There's nothing interesting there, unless we are pathologically bad at parsing the one or two markdown links. I think it's the perf tests being wonky, so let's see what the second run results are like. |
|
@sandersn Here they are:
CompilerComparison Report - main..52466
System
Hosts
Scenarios
TSServerComparison Report - main..52466
System
Hosts
Scenarios
StartupComparison Report - main..52466
System
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
@weswigham maybe you're right about the isTsserver check. The speedup is gone in the commit with the suspicious version of the check. |
|
@typescript-bot perf test this |
|
Heya @sandersn, I've started to run the perf test suite on this PR at ebcbd11. You can monitor the build here. Update: The results are in! |
|
@sandersn Here they are:
CompilerComparison Report - main..52466
System
Hosts
Scenarios
TSServerComparison Report - main..52466
System
Hosts
Scenarios
StartupComparison Report - main..52466
System
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
1. Set a global boolean when invoked from tsc. 2. Use a regex with sourceText.slice to check for @see/@link.
|
The latest commit is something we could merge, I think. However, the check for @typescript-bot perf test this |
|
Heya @sandersn, I've started to run the perf test suite on this PR at 2842d0c. You can monitor the build here. Update: The results are in! |
jakebailey
reviewed
Feb 1, 2023
src/compiler/parser.ts
Outdated
| @@ -1762,10 +1763,17 @@ namespace Parser { | |||
| return hasJSDoc ? addJSDocComment(node) : node; | |||
| } | |||
|
|
|||
| const seeLink = /@(?:see|link)/; | |||
| function shouldParseJSDoc<T extends HasJSDoc>(node: T, comment: ts.CommentRange) { | |||
| if (!(globalThis as any).isTSC) return true; | |||
There was a problem hiding this comment.
Is there a way to do this without smuggling in the boolean? How can we test that we don't break tsc?
There was a problem hiding this comment.
I mean, rather than a global context flag, best is probably to pass an option down into the parser from whatever's constructing the program, right? This way API consumers can intelligently select if they need to skip jsdoc or not.
|
@sandersn Here they are:
CompilerComparison Report - main..52466
System
Hosts
Scenarios
TSServerComparison Report - main..52466
System
Hosts
Scenarios
StartupComparison Report - main..52466
System
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Is emit time getting better because we're no longer correctly emitting jsdoc from the sources? |
ahejlsberg
reviewed
Feb 1, 2023
| let hasDeprecatedTag = false; | ||
| function addJSDocComment<T extends HasJSDoc>(node: T): T { | ||
| Debug.assert(!node.jsDoc); // Should only be called once per node | ||
| const jsDoc = mapDefined(getJSDocCommentRanges(node, sourceText), comment => JSDocParser.parseJSDocComment(node, comment.pos, comment.end - comment.pos)); | ||
| const jsDoc = mapDefined(getJSDocCommentRanges(node, sourceText), comment => shouldParseJSDoc(node, comment) && JSDocParser.parseJSDocComment(node, comment.pos, comment.end - comment.pos)); |
There was a problem hiding this comment.
Did you consider having the scanner check for @link and @see in the code in scan() that already checks for JSDoc and sets the TokenFlags.PrecedingJSDocComment flag. This could either be a new flag, or you could simply say that in .ts files you only set the TokenFlags.PrecedingJSDocComment when there are @link and @see tags. Then withJSDoc would completely skip over this additional scanning which should save you even more time.
|
Some quick replies:
|
|
The baselines wouldn't include tsc, so the maximal thing we're able to test is to skip parse of non-see/link in tests. I think that's the worry, if we don't actually have any tests which verify this. But that being said, it's just a comment, so I assume we don't actually reconstruct them at all, just copying them as is. But I do think we need to find a way to pass this information down than to only set it for the tsc executable, and then include that in the compiler test harness as an "@" option. |
|
I ran tests with skipJSDoc forced true and all the failures were in fourslash except a hilarious bug where Since performance isn't (much of) a concern in tests I think it's worthwhile to default skipJSDoc=false in tests and only have a few tests where it's true. |
|
Next step on this PR is to implement Anders' idea to have the scanner mark link/see tags the first time it sees them. |
This was referenced Feb 16, 2023
|
Superceded by #52921 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Just an experiment for now. JSDoc is still parsed
@linkor@seeAn alternative would make the parser parse lazily, and instead have the checker skip checking in the above conditions. After thinking about it, I'm not sure that's much better.