Zip @param tags and parameters together instead of looking for a name#18832
Zip @param tags and parameters together instead of looking for a name#18832ghost wants to merge 4 commits into
Conversation
sandersn
left a comment
There was a problem hiding this comment.
This is a good idea, but I don't think the current code in the binder is correct. I also want to see what shifting the work up-front does to performance. Except I don't think we have much (any?) jsdoc in our performance tests.
|
|
||
| // Clear any previous binding | ||
| for (const p of parameters) { | ||
| p.jsdocParamTags = undefined; |
There was a problem hiding this comment.
why do we need to do this? Is it because the binder is retained between edits?
There was a problem hiding this comment.
The source tree node may have been reused from a previous version of the AST so its properties must be reset.
| questionToken?: QuestionToken; // Present on optional parameter | ||
| type?: TypeNode; // Optional type annotation | ||
| initializer?: Expression; // Optional initializer | ||
| /* @internal */ jsdocParamTags?: JSDocParameterTag[]; |
There was a problem hiding this comment.
how does doing this in the binder instead of the checker affect performance? I don't understand the performance characteristics of the binder well enough to predict, so maybe a perf test is in order.
| } | ||
|
|
||
| let paramIndex = 0; | ||
| for (const tag of getJSDocTags(parent)) { |
There was a problem hiding this comment.
This is going to be O(mn) where m = |tags| and n = |params|. That's because, for each tag, we examine every parameter starting at 0. Is this necessary? I think it would suffice to increment paramIndex until a single matching tag is found.
Even better, you could iterate over the parameters instead and keep a non-restarting index into the tags. You can give up when the tags run out.
zip :: [Tag] -> [Param] -> [Param]
zip (t:tags) (p:params) | name t == name p = p { tags = Just t } : zip tags params
zip (t:tags) params = zip tags params
zip [] params = params
zip tags [] = []Note that the Haskell version doesn't side-effect, it just assumes there is a field tags :: Maybe Tag that is set to Nothing when this function is called.
let tagIndex = 0;
for (const param of parameters) {
while (tags[tagIndex].name.escapedText !== param.name.escapedText && tagIndex < tags.length) {
tagIndex++;
}
if (tagIndex >= tags.length) {
break;
}
if (tags[tagIndex].name.escapedText === param.name.escapedText) {
param.jsdocParamTags = append(param.jsdocParamTags, tags[tagIndex]);
}
}There was a problem hiding this comment.
We want to keep supporting out-of-order @param tags, but in the case where they are in order this will be O(n) because we start looking at the previous paramIndex so usually we only go through the loop once.
There was a problem hiding this comment.
I see. I misunderstood the const startParamIndex = paramIndex. This is better than what I proposed, then.
|
@Andy-MS can you please refresh and address the comments above. |
|
@Andy-MS I've seen destructuring with JSDoc in code bases like webpack, which would benefit from this. Can you bring this back up to date and then I can take another look? |
|
When running our existing performance tests, the binder got slower for Angular, but it got faster for Compiler w/Unions. But everything got a lot faster for the Compiler w/Unions, which doesn't make sense, since jsdoc params shouldn't be checked for types at all in typescript. I'm going to try adding a javascript project to the performance tests and see if I can learn anything. |
|
I ran another couple of perf tests, the last with me away from the computer. The binder is definitely slower for Angular -- about 10%. The binder runs in about a second, so it's easy to get swamped in the noise; TFS is slower but just barely significant for exactly that reason. Everything is a little bit slower, but not enough to be significant. I think it may becase this change double caches: getJSDocTags caches into jsdocCache. Then during binding, bindJSDocParameters caches into jsdocParamTags ... after it calls getJSDocTags. That might be enough to slow everything down a little bit. |
|
Thanks for your contribution. This PR has not been updated in a while and cannot be automatically merged at the time being. For housekeeping purposes we are closing stale PRs. If you'd still like to continue working on this PR, please leave a message and one of the maintainers can reopen it. |
Avoids O(n^2) lookup in the usual case that
@paramtags are in the same order as parameters.As a side-effect, we can now bind a
@paramtag to a bind expression if they have the same index.Also fixes #17217.