@link support, second try #43312
@link support, second try #43312
Conversation
|
Thanks for the PR! It looks like you've changed the TSServer protocol in some way. Please ensure that any changes here don't break consumers of the current TSServer API. For some extra review, we'll ping @sheetalkamat, @amcasey, @mjbvz, @minestarks for you. Feel free to loop in other consumers/maintainers if necessary |
typescript-bot
added
Author: Team
For Uncommitted Bug
|
@typescript-bot run dt |
|
Heya @sandersn, I've started to run the perf test suite on this PR at 053b7dc. You can monitor the build here. Update: The results are in! |
|
@sandersn Here they are:Comparison Report - master..43312
System
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Here are the results for node 14 (the node 12 results are basically identical and I didn't look at node 10):
Memory usage is stable across runs, so these results are highly statistically significant. |
sandersn
requested review from
amcasey and
andrewbranch
and removed request for
andrewbranch
|
After looking closer at the results and doing some local testing, it looks like 1000 jsdoc = 1 MB of memory increase. I think that's fine. The number is alarming for activex-libreoffice because it has so much jsdoc and because checking is so easy. I recommend taking this change. |
|
@sandersn It sounds like you're saying that each jsdoc comment uses 1KB more memory than its string representation - is that right? That seems kind of high, doesn't it? How many objects is each comment turning into? |
|
The common case of a jsdoc comment with no links should create an additional NodeArray (an array instance-patched with 2 properties and optionally (rarely) 2 more) that contains a single JSDocText (a Node instance, which has 9 properties and optionally 12 more), which contains the string that was originally there. I did a quick experiment, which turned into a lot of measurement, to find the likelihood of bugs here. I compared master with this branch, as well as a branch that doesn't change the parser except to add a singleton NodeArray containing a JSDocText. It's at jsdoc-simple-parse-EXPERIMENT -- it's an obvious subset of this PR. It does not parse Edit: After discussing ideas with @amcasey, I made the AST comment type: Memory usage is an average of 7 runs. activex-libreoffice16,000 jsdocs and 6,000 master: 226k nodes, 114.4 MB (with a surprising amount of variance, much more than the +-500k I'm used to seeing) angular 23,000 jsdocs and 500 master: 934k nodes, 445 MB compiler-unions1,000 jsdocs, 1 There is quite a bit of variance in memory usage for compiler-unions. Maybe I'm seeing the effect of the new union cache? |
|
I might be blanking, but why do we do any of this work in tsc scenarios? Isn't |
|
The way it's implemented now gives jsdoc texts a full AST parse. That form makes it much easier to produce displayparts for the editor. |
|
Right, but couldn't we skip that extra parsing in tsc scenarios? |
| const comments: string[] = []; | ||
| let linkEnd: number; | ||
| let commentsPos: number | undefined; | ||
| let comments: string[] = []; |
There was a problem hiding this comment.
Not new, but why is comments accumulated as an array of parts and joined later, rather than simply accumulated as a string?
There was a problem hiding this comment.
Habit from immutable-string languages without optimisations for +=. I'm not sure if JS is still one of these languages or not.
There was a problem hiding this comment.
I'm pretty sure I remember reading that += is the fastest way to accumulate a string in JS, but I don't have a source.
There was a problem hiding this comment.
I'm going to try this in a separate PR.
src/compiler/types.ts
Outdated
| @@ -3133,13 +3135,24 @@ namespace ts { | |||
| readonly kind: SyntaxKind.JSDocComment; | |||
| readonly parent: HasJSDoc; | |||
| readonly tags?: NodeArray<JSDocTag>; | |||
| readonly comment?: string; | |||
| readonly comment?: NodeArray<JSDocText | JSDocLink>; | |||
There was a problem hiding this comment.
If we decided we were sufficiently bothered by the increased memory usage, could this be string | NodeArray? Alternatively, could we have JSDoc and JSDocWithLinks?
|
The common case experiment worked out pretty well, except for activex-libreoffice, which has really weird characteristics already. I'm going to fix a bug in services that I introduced, push the commit, and run perf tests on it to make sure that it doesn't slow things down a lot. |
|
@typescript-bot perf test this |
|
Heya @sandersn, I've started to run the perf test suite on this PR at 4db5d97. You can monitor the build here. Update: The results are in! |
|
@sandersn Here they are:Comparison Report - master..43312
System
Hosts
Scenarios
Developer Information: |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Perf looks good; only angular's memory usage goes up. Edit: DT is fine too, but I only ran that as insurance. |
update typescript to v4.3.5 add type annotations to methods that have an implicit any type in TS 4.3 - for more information, see microsoft/TypeScript#26623 map TS 4.3+ `JSDocTagInfo` to `CompilerJsDocTagInfo` - for more information, see microsoft/TypeScript#43312 add type guard for `TestingSystem`. in TS 4.3, `Object.defineProperties` is now generic, and returns the same type `T` as it's first argument. adding a typeguard allows consumers of this method to ensure they're receiving a `TestingSystem` object enable `noImplicitOverride` compiler flag
Same as #41877, but with an additional commit to fix parsing unterminated
@linktags that go to the end of a comment.Note that local runs of activex-libreoffice show memory usage 100 MB -> 118 MB and 226k -> 290k nodes, an 18% increase in memory and 28% increase in node count. That's quite a bit, although activex-libreoffice uses jsdoc and
@linkheavily.I'm going to make sure that DT succeeds with this build and also look at memory use from the perf tests.