Skip to content

Conversation

@0101
Copy link
Contributor

@0101 0101 commented Nov 10, 2022

Finding references can be quite slow because it requires full type checking results. This is probably the main obstacle for a working "rename" functionality in large projects.

My idea is to have a preliminary look at a set of all the identifiers in the file and see if the symbol we're looking for is in the set - and only if it is do the potentially expensive checking to actually find if its referenced.

It's not going to speed up 100% of the cases (like when the symbol is referenced in the very last file, requiring to check everything else anyway, or when you're renaming some identifier like x which you use in a lot of files), but it should speed up a lot of them.

When trying this on random symbols from FSharp solution in many cases there was quite noticeable (5-10x) speed-up, making renaming and finding references pretty usable. I'm trying to put together some proper benchmarks.

How to test this locally

  1.  .\Build.cmd -pack -c release
  2. Go to artifacts\VSSetup\Release
  3. Double click VisualFSharpDebug.vsix to install it in VS
  4. Start VS, go to Tools -> Options -> Text Editor -> F# -> Performance
  5. Check Enable fast find references & rename (experimental)
    image
  6. Try renaming stuff or Find all references

Synthetic benchmarks

Finding all references to a symbol defined in first file. The project has 50 files with 1400 LOC each. There are no other identifiers with the same name as the symbol.

  • Best case = symbol is only referenced in the second file
  • Medium case = symbol is referenced in 3 files in the middle
  • Worst case = symbol is referenced in every single file

The most interesting cases are best and medium case when the cache is empty (e.g. first file was just edited and invalidated everything.) where we get roughly 20x and 2x speed-up respectively.

Method FastFindReferences EmptyCache Speed-up Mean Error StdDev Gen0 Gen1 Gen2 Allocated
FindAllReferences_BestCase False False 202.20 ms 3.946 ms 4.846 ms - - - 6.21 MB
FindAllReferences_MediumCase False False 199.17 ms 2.316 ms 2.166 ms - - - 7.11 MB
FindAllReferences_WorstCase False False 227.39 ms 3.100 ms 2.900 ms - - - 7.03 MB
FindAllReferences_BestCase False True 8,812.77 ms 36.646 ms 32.485 ms 22000.0000 11000.0000 2000.0000 7867.5 MB
FindAllReferences_MediumCase False True 8,931.69 ms 22.701 ms 20.124 ms 22000.0000 11000.0000 2000.0000 7868.34 MB
FindAllReferences_WorstCase False True 8,910.66 ms 23.800 ms 22.262 ms 23000.0000 11000.0000 3000.0000 7860.7 MB
FindAllReferences_BestCase True False 2.2 93.52 ms 1.795 ms 1.679 ms - - - 176.35 MB
FindAllReferences_MediumCase True False 2.0 97.45 ms 1.846 ms 1.636 ms - - - 176.72 MB
FindAllReferences_WorstCase True False 1.0 230.93 ms 3.318 ms 3.103 ms - - - 179.01 MB
FindAllReferences_BestCase True True 18.0 489.87 ms 7.065 ms 6.609 ms 1000.0000 - - 444.34 MB
FindAllReferences_MediumCase True True 1.9 4,721.42 ms 23.230 ms 21.729 ms 13000.0000 6000.0000 2000.0000 4315.97 MB
FindAllReferences_WorstCase True True 1.0 8,844.31 ms 30.303 ms 28.345 ms 22000.0000 11000.0000 2000.0000 8033.77 MB

@vzarytovskii
Copy link
Member

8Gb worst case looks concerning

@0101
Copy link
Contributor Author

0101 commented Nov 11, 2022

8Gb worst case looks concerning

Well worst case performs the same as what we have now. Although identifiers are still being collected even when FastFindReferences is false, not sure how much impact that has. Have run the benchmark against main.

@0101
Copy link
Contributor Author

0101 commented Nov 11, 2022

I guess there's an extra ~180 MB being allocated, which can be seen even when cache is not emptied. But that still wouldn't be a bad trade-off even if we couldn't get rid of that.

@T-Gro
Copy link
Member

T-Gro commented Nov 15, 2022

Would be good to calculate and highlight the ratio between old & new results, to make it explicit.

As far as I am concerned, this is really great - if we are certain this does have full recall and no missing hits (it is fine to have suboptimal precision, since it is just a heuristics and the precise search still happens)

@0101
Copy link
Contributor Author

0101 commented Nov 15, 2022

@T-Gro well the benchmarks are not super representative of real projects. But I might just do some on finding various symbols in FCS project.

And yes we'd need to ensure that it doesn't miss anything. Right now it's comparing parsed IDENT to Symbol.DisplayName. Not sure yet if this will catch everything, have to look into the names more.

@dsyme
Copy link
Contributor

dsyme commented Nov 16, 2022

This looks like fabulous work!

@0101
Copy link
Contributor Author

0101 commented Nov 17, 2022

Thanks @dsyme. What do you think about exposing the identifiers on ParsedInput? Would we be ok with that or should we look for another way?

@0101
Copy link
Contributor Author

0101 commented Nov 22, 2022

Found out it wasn't working for names in backticks, operators and attributes. Fixed those and added tests, looking for other cases where it might fail...

@vzarytovskii
Copy link
Member

Found out it wasn't working for names in backticks, operators and attributes. Fixed those and added tests, looking for other cases where it might fail...

I think it's worth inserting it with the flag disabled by default and test it on the compiler, as we work on it. And push fixes as we find any issues. wdyt?

@0101
Copy link
Contributor Author

0101 commented Nov 22, 2022

I moved the flag from creating FSharpChecker to the FindBackgroundReferencesInFile method so changing the setting now doesn't require a restart.

@0101 0101 changed the title Proof-of-concept faster finding references Faster finding references Nov 23, 2022
@0101 0101 marked this pull request as ready for review November 23, 2022 11:01
@0101 0101 requested a review from a team as a code owner November 23, 2022 11:01
T-Gro
T-Gro previously approved these changes Nov 23, 2022
vzarytovskii
vzarytovskii previously approved these changes Nov 23, 2022
psfinaki
psfinaki previously approved these changes Nov 24, 2022
Copy link
Contributor

@psfinaki psfinaki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good stuff, looking forward to have this in VS!

@T-Gro
Copy link
Member

T-Gro commented Nov 24, 2022

Let's merge this, or?

@0101 0101 dismissed stale reviews from psfinaki, vzarytovskii, and T-Gro via ca366b9 November 24, 2022 13:23
@0101
Copy link
Contributor Author

0101 commented Nov 24, 2022

@T-Gro @psfinaki @vzarytovskii please re-review/approve, I addressed the PR comments.

@0101 0101 enabled auto-merge (squash) November 24, 2022 13:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

6 participants