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

Renaming private declarations takes 40+ seconds #57031

Open
FMorschel opened this issue Nov 5, 2024 · 11 comments
Open

Renaming private declarations takes 40+ seconds #57031

FMorschel opened this issue Nov 5, 2024 · 11 comments
Labels
area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. P3 A lower priority bug or feature request type-enhancement A request for a change that isn't a bug

Comments

@FMorschel
Copy link
Contributor

While working at https://dart-review.googlesource.com/c/sdk/+/392860 for #49294, I was refactoring and needed to rename a private method for AddConst.

The method was used only once and since it is a file with no parts the analysis and renaming for this should be really fast.

image
image

After talking with @DanTup on Discord he mentioned that this is actually caused by the creation of an index that for future renamings would decrease the waiting time.

I was wondering if we could ever do some form of work to help users with this long waiting time for renaming simple things. I have two suggestions which basically overlap:

  1. Creating that index in the background hopefully before it is needed in a way that doesn't slow down things the user does care about (suggested by Dan).
  2. Basically the above but with a small tweak. I think this could be started whenever the user did request to rename something but we could actually try and leave that work running separately after we had solved the single rename request. I'm not sure how exactly we could do this but I thought of something like:
  • User requests to rename something
  • We calculate every reference to it that needs to be renamed
  • Return that result but also trigger the background process to calculate the index for everything else
@dart-github-bot
Copy link
Collaborator

Summary: Renaming a private method in a Dart file with no parts takes over 40 seconds, significantly slowing down refactoring. The issue is caused by the creation of an index for future renamings, which is currently done synchronously. The user suggests creating the index in the background to avoid impacting user workflow.

@dart-github-bot dart-github-bot added area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. triage-automation See https://github.com/dart-lang/ecosystem/tree/main/pkgs/sdk_triage_bot. type-enhancement A request for a change that isn't a bug labels Nov 5, 2024
@FMorschel
Copy link
Contributor Author

Just suggesting this because in my case I could've simply used find/replace and that would have been way faster. And people who experience this kind of long waiting might use this instead because of this even though there may be some downsides like needing to take a look at Strings and similar things for correct handling of it.

@bwilkerson
Copy link
Member

@scheglov for verification

Creating that index in the background ...

I think we already do. Last I looked at it, the index is created (and updated) every time we analyze a library cycle, which means that after the initial analysis of everything that's in the workspace we should have everything we need.

@scheglov
Copy link
Contributor

scheglov commented Nov 5, 2024

Yes, we compute index when we resolve a Dart file.

We do search across all libraries of a Pub package, including dependencies. So, we need to index all of them. Even if rename never updates dependencies, the search engine, and the index are used for both search and rename. Because rename is search essentially. So, when you do search or rename the first time, we discover and index all dependencies.

@FMorschel
Copy link
Contributor Author

What is happening here then? There is no subprocess or something to help us see inner timings so I'm unsure of what could be happening.

image

@scheglov
Copy link
Contributor

scheglov commented Nov 5, 2024

Probably. Yes, we did not have need to break this down into smaller performance operations.

@DanTup
Copy link
Collaborator

DanTup commented Nov 6, 2024

@FMorschel can you reliably reproduce this? I just tried to repro to see if I could get some approx timings with the CPU profiler to confirm the time is where we think (I wondered if some work could be skipped when we know we're only searching for privates?), so I tried:

  • Adding a private member to class AddConst like bool _foo() => false;
  • Restarted VS Code
  • Waited for initial analysis to complete (from your screenshot, I don't think you did this during initial analysis)
  • Renamed the method

It wasn't fast, but it was only 2-3s which is very different to what you see. That doesn't feel like the kind of difference that differences between our hardware so I think I might not be reproducing the same thing.

@FMorschel
Copy link
Contributor Author

I can. I'm unsure about the real waiting time, but last night, I was working on an assist and renamed the test class (it is just used locally, but it is public), which also took a long time. I'm unsure of what I can do to help identify the problem here. If you have any ideas I'm open to them.

@DanTup
Copy link
Collaborator

DanTup commented Nov 6, 2024

@FMorschel sorry I should've been clearer what I was asking... Can you describe the precise steps so I can see if I can repro in the same way here? Can you also confirm the SDK version you're using for analysis (eg. the one VS Code is selecting... probably your value from dart.sdkPath), and whether you're running the server from source (dart.analyzerPath) or from the snapshot inside that SDK. Thanks!

@devoncarew devoncarew removed the triage-automation See https://github.com/dart-lang/ecosystem/tree/main/pkgs/sdk_triage_bot. label Nov 6, 2024
@FMorschel
Copy link
Contributor Author

Can you also confirm the SDK version you're using for analysis

Dart 3.7.0-edge.3a895e28b44aacd08bd9055f05c16f14078eb067 (main) (Wed Nov 6 10:23:38 2024 +0000) on "windows_x64"

and whether you're running the server from source or from the snapshot inside that SDK

The snapshot =D


  • Open the SDK project.
  • Open pkg\analysis_server\test\src\services\correction\fix\add_missing_parameter_required_test.dart
  • Rename AddMissingParameterRequiredTest to AAddMissingParameterRequiredTest

image
image

About 20s in, I do see that the IDE shows "Analyzing..." on the status bar so some time for that can be reanalyzing files but still. If you now undo this and rename I don't get more than 85ms to rename the same class or AddMissingParameterRequiredTest_Workspacee (same file).

If I leave the project open for some time and try to rename it again, it can take ~3s as you mentioned so that is consistent.

@FMorschel
Copy link
Contributor Author

FMorschel commented Nov 7, 2024

Also, if you think that solving for private declarations is easier you can take a look at AddConst._declarationListIsFullyConst in pkg/analysis_server/lib/src/services/correction/dart/add_const.dart and rename it. Since that is what made me open this issue and is now merged.

@scheglov scheglov added the P3 A lower priority bug or feature request label Nov 8, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-analyzer Use area-analyzer for Dart analyzer issues, including the analysis server and code completion. P3 A lower priority bug or feature request type-enhancement A request for a change that isn't a bug
Projects
None yet
Development

No branches or pull requests

6 participants