Skip to content

Return all symbols in navto for empty string pattern #55550

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

Merged
merged 3 commits into from
Aug 30, 2023
Merged

Conversation

gabritto
Copy link
Member

Fixes #55483.

@typescript-bot typescript-bot added Author: Team For Milestone Bug PRs that fix a bug with a specific milestone labels Aug 28, 2023
@DanielRosenwasser
Copy link
Member

DanielRosenwasser commented Aug 28, 2023

Does navto do an after-the-fact filter today? Or does it filter as it aggregates symbols? Also curious if you've tried this on slightly bigger codebases like VS Code or TypeScript itself (for both Go To Symbol in Editor and Go To Symbol in Workspace).

@gabritto
Copy link
Member Author

Does navto do an after-the-fact filter today? Or does it filter as it aggregates symbols? Also curious if you've tried this on slightly bigger codebases like VS Code or TypeScript itself (for both Go To Symbol in Editor and Go To Symbol in Workspace).

Do you mean if vscode is going to filter the navto results TS Server sends? If so, it doesn't. I'll wait to merge this until we decide on the changes needed on the editor side.

@gabritto
Copy link
Member Author

We might also want to consider doing #48194, but I don't know what was discussed/decided about it the last time it was brought up.

@gabritto
Copy link
Member Author

@typescript-bot perf test this

@typescript-bot
Copy link
Collaborator

typescript-bot commented Aug 28, 2023

Heya @gabritto, I've started to run the regular perf test suite on this PR at 8d43af7. You can monitor the build here.

Update: The results are in!

@typescript-bot
Copy link
Collaborator

@gabritto
The results of the perf run you requested are in!

Here they are:

Compiler

Comparison Report - baseline..pr
Metric baseline pr Delta Best Worst p-value
Angular - node (v16.17.1, x64)
Memory used 300,256k (± 0.00%) 300,264k (± 0.01%) ~ 300,242k 300,297k p=0.630 n=6
Parse Time 3.03s (± 0.49%) 3.03s (± 0.13%) ~ 3.03s 3.04s p=1.000 n=6
Bind Time 0.93s (± 0.00%) 0.93s (± 0.00%) ~ 0.93s 0.93s p=1.000 n=6
Check Time 9.28s (± 0.16%) 9.28s (± 0.26%) ~ 9.25s 9.32s p=0.739 n=6
Emit Time 7.62s (± 0.39%) 7.62s (± 0.10%) ~ 7.61s 7.63s p=1.000 n=6
Total Time 20.86s (± 0.21%) 20.87s (± 0.12%) ~ 20.84s 20.91s p=0.809 n=6
Compiler-Unions - node (v16.17.1, x64)
Memory used 193,934k (± 0.02%) 194,458k (± 0.66%) ~ 193,888k 197,096k p=0.630 n=6
Parse Time 1.58s (± 0.33%) 1.58s (± 0.33%) ~ 1.58s 1.59s p=1.000 n=6
Bind Time 0.80s (± 0.00%) 0.80s (± 0.00%) ~ 0.80s 0.80s p=1.000 n=6
Check Time 9.91s (± 0.50%) 9.93s (± 0.78%) ~ 9.84s 10.05s p=0.936 n=6
Emit Time 2.74s (± 0.27%) 2.74s (± 0.15%) ~ 2.73s 2.74s p=0.389 n=6
Total Time 15.03s (± 0.37%) 15.05s (± 0.52%) ~ 14.96s 15.17s p=0.872 n=6
Monaco - node (v16.17.1, x64)
Memory used 347,148k (± 0.01%) 347,140k (± 0.01%) ~ 347,079k 347,181k p=0.873 n=6
Parse Time 2.69s (± 0.31%) 2.69s (± 0.23%) ~ 2.68s 2.70s p=0.226 n=6
Bind Time 0.99s (± 0.00%) 0.99s (± 0.41%) ~ 0.98s 0.99s p=0.405 n=6
Check Time 7.90s (± 0.12%) 7.91s (± 0.21%) ~ 7.89s 7.94s p=0.437 n=6
Emit Time 4.26s (± 0.38%) 4.25s (± 0.32%) ~ 4.24s 4.27s p=0.804 n=6
Total Time 15.83s (± 0.14%) 15.84s (± 0.20%) ~ 15.79s 15.88s p=0.686 n=6
TFS - node (v16.17.1, x64)
Memory used 301,153k (± 0.00%) 301,157k (± 0.00%) ~ 301,128k 301,166k p=0.261 n=6
Parse Time 2.18s (± 0.56%) 2.18s (± 0.58%) ~ 2.16s 2.19s p=0.452 n=6
Bind Time 1.11s (± 0.46%) 1.11s (± 0.00%) ~ 1.11s 1.11s p=0.174 n=6
Check Time 7.23s (± 0.34%) 7.22s (± 0.38%) ~ 7.20s 7.26s p=0.514 n=6
Emit Time 3.99s (± 0.19%) 3.98s (± 0.49%) ~ 3.95s 4.01s p=0.118 n=6
Total Time 14.51s (± 0.23%) 14.49s (± 0.27%) ~ 14.45s 14.56s p=0.366 n=6
material-ui - node (v16.17.1, x64)
Memory used 479,463k (± 0.00%) 479,467k (± 0.00%) ~ 479,443k 479,479k p=0.470 n=6
Parse Time 3.15s (± 0.20%) 3.15s (± 0.24%) ~ 3.14s 3.16s p=0.718 n=6
Bind Time 0.91s (± 0.00%) 0.91s (± 0.00%) ~ 0.91s 0.91s p=1.000 n=6
Check Time 17.76s (± 0.23%) 17.77s (± 0.34%) ~ 17.66s 17.84s p=0.747 n=6
Emit Time 0.00s (± 0.00%) 0.00s (± 0.00%) ~ 0.00s 0.00s p=1.000 n=6
Total Time 21.82s (± 0.21%) 21.82s (± 0.30%) ~ 21.71s 21.91s p=0.687 n=6
xstate - node (v16.17.1, x64)
Memory used 542,833k (± 0.01%) 542,848k (± 0.01%) ~ 542,782k 542,957k p=0.936 n=6
Parse Time 3.70s (± 0.24%) 3.70s (± 0.32%) ~ 3.69s 3.72s p=0.672 n=6
Bind Time 1.40s (± 4.57%) 1.44s (± 3.40%) ~ 1.34s 1.46s p=0.144 n=6
Check Time 3.23s (± 2.97%) 3.18s (± 2.04%) ~ 3.13s 3.31s p=0.685 n=6
Emit Time 0.08s (± 0.00%) 0.08s (± 4.99%) ~ 0.08s 0.09s p=0.405 n=6
Total Time 8.42s (± 0.38%) 8.40s (± 0.37%) ~ 8.38s 8.46s p=0.465 n=6
System info unknown
Hosts
  • node (v16.17.1, x64)
Scenarios
  • Angular - node (v16.17.1, x64)
  • Compiler-Unions - node (v16.17.1, x64)
  • Monaco - node (v16.17.1, x64)
  • TFS - node (v16.17.1, x64)
  • material-ui - node (v16.17.1, x64)
  • xstate - node (v16.17.1, x64)
Benchmark Name Iterations
Current pr 6
Baseline baseline 6

tsserver

Comparison Report - baseline..pr
Metric baseline pr Delta Best Worst p-value
Compiler-UnionsTSServer - node (v16.17.1, x64)
Req 1 - updateOpen 2,493ms (± 0.12%) 2,490ms (± 0.20%) ~ 2,484ms 2,496ms p=0.746 n=6
Req 2 - geterr 5,933ms (± 0.20%) 5,941ms (± 0.11%) ~ 5,934ms 5,950ms p=0.172 n=6
Req 3 - references 344ms (± 0.44%) 343ms (± 0.45%) ~ 342ms 346ms p=0.502 n=6
Req 4 - navto 278ms (± 0.84%) 277ms (± 0.15%) ~ 276ms 277ms p=0.056 n=6
Req 5 - completionInfo count 1,356 (± 0.00%) 1,356 (± 0.00%) ~ 1,356 1,356 p=1.000 n=6
Req 5 - completionInfo 87ms (± 8.65%) 88ms (± 8.55%) ~ 76ms 94ms p=1.000 n=6
CompilerTSServer - node (v16.17.1, x64)
Req 1 - updateOpen 2,615ms (± 0.19%) 2,627ms (± 0.43%) ~ 2,612ms 2,639ms p=0.077 n=6
Req 2 - geterr 4,766ms (± 0.34%) 4,769ms (± 0.15%) ~ 4,762ms 4,780ms p=0.936 n=6
Req 3 - references 352ms (± 0.82%) 350ms (± 0.12%) ~ 350ms 351ms p=0.474 n=6
Req 4 - navto 270ms (± 0.41%) 269ms (± 0.56%) ~ 267ms 271ms p=0.451 n=6
Req 5 - completionInfo count 1,518 (± 0.00%) 1,518 (± 0.00%) ~ 1,518 1,518 p=1.000 n=6
Req 5 - completionInfo 79ms (± 0.52%) 78ms (± 3.01%) ~ 73ms 79ms p=0.248 n=6
xstateTSServer - node (v16.17.1, x64)
Req 1 - updateOpen 2,714ms (± 0.23%) 2,714ms (± 0.17%) ~ 2,709ms 2,721ms p=1.000 n=6
Req 2 - geterr 1,929ms (± 2.74%) 1,935ms (± 1.96%) ~ 1,884ms 1,968ms p=0.936 n=6
Req 3 - references 137ms (± 2.05%) 135ms (± 2.34%) ~ 132ms 141ms p=0.371 n=6
Req 4 - navto 353ms (± 0.44%) 353ms (± 0.21%) ~ 352ms 354ms p=0.934 n=6
Req 5 - completionInfo count 2,071 (± 0.00%) 2,071 (± 0.00%) ~ 2,071 2,071 p=1.000 n=6
Req 5 - completionInfo 317ms (± 1.93%) 317ms (± 1.76%) ~ 312ms 327ms p=0.868 n=6
System info unknown
Hosts
  • node (v16.17.1, x64)
Scenarios
  • CompilerTSServer - node (v16.17.1, x64)
  • Compiler-UnionsTSServer - node (v16.17.1, x64)
  • xstateTSServer - node (v16.17.1, x64)
Benchmark Name Iterations
Current pr 6
Baseline baseline 6

Startup

Comparison Report - baseline..pr
Metric baseline pr Delta Best Worst p-value
tsc-startup - node (v16.17.1, x64)
Execution time 156.25ms (± 0.22%) 156.21ms (± 0.19%) ~ 154.45ms 158.48ms p=0.065 n=600
tsserver-startup - node (v16.17.1, x64)
Execution time 230.25ms (± 0.13%) 231.03ms (± 0.14%) +0.78ms (+ 0.34%) 229.45ms 235.14ms p=0.000 n=600
tsserverlibrary-startup - node (v16.17.1, x64)
Execution time 234.89ms (± 0.12%) 236.12ms (± 0.14%) +1.22ms (+ 0.52%) 234.68ms 242.06ms p=0.000 n=600
typescript-startup - node (v16.17.1, x64)
Execution time 236.37ms (± 0.16%) 236.17ms (± 0.13%) -0.20ms (- 0.08%) 233.76ms 240.28ms p=0.000 n=600
System info unknown
Hosts
  • node (v16.17.1, x64)
Scenarios
  • tsc-startup - node (v16.17.1, x64)
  • tsserver-startup - node (v16.17.1, x64)
  • tsserverlibrary-startup - node (v16.17.1, x64)
  • typescript-startup - node (v16.17.1, x64)
Benchmark Name Iterations
Current pr 6
Baseline baseline 6

Developer Information:

Download Benchmarks

@gabritto
Copy link
Member Author

@gabritto The results of the perf run you requested are in!

Here they are:

I just remembered the benchmark sends a navto request with some pre-defined string... So the perf tests are not relevant here. But I tested this locally and there isn't a delay on the vscode codebase, but for the TS codebase there's a ~1.5s delay between the command and the results showing up.

@DanielRosenwasser
Copy link
Member

No, I was asking if it's the case that running navTo with "getTypeFrom" will walk across the whole project and then filter the symbols based on getTypeFrom, or if the filtering is done during the walk itself.

@gabritto
Copy link
Member Author

gabritto commented Aug 28, 2023

No, I was asking if it's the case that running navTo with "getTypeFrom" will walk across the whole project and then filter the symbols based on getTypeFrom, or if the filtering is done during the walk itself.

It'll walk across all files in all the projects, collect all named declarations (if they haven't been computed yet), and then look at every named declaration to see if it matches "getTypeFrom".
So yes, the filtering is done after-the-fact.

@gabritto
Copy link
Member Author

Update: I did some perf testing locally of how long it takes for TS Server to process navto request in the TS codebase with no search of empty string vs. a single letter "a" vs. "node".

The first ever navto request takes a long time (0.5s for me), because the named declarations of each file are not cached yet, but other than that:

  • "" takes ~0.115s
  • "a" takes ~0.112s
  • "node" takes ~0.8s

So overall I think this looks acceptable as-is.

At first I thought this empty string search was a bit slow because I was testing it on vscode directly, and there seems to be an added delay on vscode that is at times inconsistent, but at least to me it shows up not only for the empty string search, but for other navto searches as well. So I think this is no worse than what we have today.

@gabritto gabritto merged commit cbadc78 into main Aug 30, 2023
@gabritto gabritto deleted the gabritto/issue55483 branch August 30, 2023 22:30
snovader pushed a commit to EG-A-S/TypeScript that referenced this pull request Sep 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Author: Team For Milestone Bug PRs that fix a bug with a specific milestone
Projects
None yet
Development

Successfully merging this pull request may close these issues.

navto returns no results for empty strings
4 participants