Skip to content
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

Fix renames and reference counts by using the compiler-generated memory-mapped indexes #945

Merged
merged 30 commits into from Jun 1, 2022

Conversation

baronfel
Copy link
Contributor

@baronfel baronfel commented May 14, 2022

We use the compiler's item key stores to power reference identification, when in turn powers rename replacement. This seems to work incredibly fast.

This is implemented by mimicking the VS integration:

  • Find the symbol at the invocation point
  • Find all references to that symbol in the workspace
    • This involves finding all files in all projects that could feasibly depend on this symbol and aggregating the references to the symbol in each one
  • use those reference ranges to issue textEdits

As a result of these changes, the background services were no longer used, so I removed them. This should result in fairly large memory/CPU savings for users that had them enabled (since you basically pay the price for loading projects/assemblies twice with them on).

Fixes ionide/ionide-vscode-fsharp#1690 and ionide/ionide-vscode-fsharp#1265

TODO:

  • Check that typechecking of dependent files works after this: When A is changed, B should be triggered. First edit to A should definitely work, but verify that for many subsequent edits to A, corresponding edits to B are checked. (Cross-file, same project).
  • Check the same for cross-project typechecking
  • Check the rename of fully-qualified identifiers (usage of A.B.C I think renames the entire range if you rename C, which is bad)

@baronfel baronfel force-pushed the remove-background-service branch 2 times, most recently from 0049591 to 2814e4d Compare May 14, 2022 19:57
@baronfel
Copy link
Contributor Author

I was able to turn on some rename tests that cover same-project renames and they green'd up nicely. I should be able to expand those tests to show cross-project renames as well. After that, I can dig into the dependent-file checking part of this PR.

@baronfel
Copy link
Contributor Author

Cross-project renames are working great now 👍 . Next steps are the validation of dependent-file checking. @Krzysztof-Cieslak mentioned today that one of the purposes of the background services was to make sure that on subsequent edits to a file A.fs in A.fsproj, that B.fs in B.fsproj was re-checked. We should have some set of tests/scenarios verifying this behavior as a baseline.

@Krzysztof-Cieslak
Copy link
Member

In the first pass let's make sure that single project dependent files are working correctly since this is thing that was broken for me last time I've tried to disable background service

The cross project is kinda stretch goal - the feature exist in background service, but is disabled due to performance considerations

@baronfel
Copy link
Contributor Author

Current state is

  • renames/reference lenses are good across platforms now
  • the dependent-file checking tests are universally failing, but I believe this is because of timing issues in the tests themselves - I've done manual validation of the dependent-file checks and it seems to work as I'd expect. I want to do a more thorough manual test thread here though.
  • several windows tests are failing due to what I assume is concurrent access of a locked file - will investigate.

@baronfel
Copy link
Contributor Author

Alright, the only tests failing are the newly-added dependent-file ones. I'm....waffling on fixing those immediately - if manual testing shows that features seem to be working, then I'm less inclined to invest the effort right now, when the rest of this PR represents a large reduction in CPU/memory usage rates.

@baronfel
Copy link
Contributor Author

baronfel commented May 27, 2022

I figured out a way to fix the bad renames that the FCS APIs give us, I'll add some tests to cover it. Dependent typechecking does work within a project, but not across projects.

@baronfel
Copy link
Contributor Author

After chatting with @Krzysztof-Cieslak we don't have cross-file checking at the moment anyway, so that will become a stretch goal, for a later PR.

Here's what it looks like now:
actual-rename
fast-file-checking

I'm super happy with this, I want to add a test around the qualified-useage-rename scenario to cover my bases, and fix the hang in the tests introduced by the last commit, but then this is good to go.

@baronfel
Copy link
Contributor Author

Green! And the failures on windows are a race condition on restoring the underlying test project. We should be able to fix that by using temp dirs.

@baronfel baronfel marked this pull request as ready for review May 30, 2022 02:25
@baronfel
Copy link
Contributor Author

baronfel commented Jun 1, 2022

Alright, going with this now :)

@baronfel baronfel merged commit 85ca7e8 into ionide:main Jun 1, 2022
@baronfel baronfel deleted the remove-background-service branch June 1, 2022 20:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

Renaming symbols is broken in v6.0.0
2 participants