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

Subscribe to analysis.navigation for visible editors and use the results for instant navigation #3620

Closed
JoeyScarr opened this issue Oct 18, 2021 · 8 comments
Labels
in editor Relates to code editing or language features is enhancement
Milestone

Comments

@JoeyScarr
Copy link
Contributor

In my company's codebase, code navigation is intolerably slow, to the point where most devs just use global find instead of the jump-to-def/find-all-references functionality.

Is there some reason why Dart-Code doesn't subscribe to analysis.navigation events from the language server? FWIW Dart for Webstorm does this and uses the results for code navigation, making it instant.

I have a hacked up version of Dart-Code locally that does this and it vastly improves the experience for me. Happy to send a PR if anyone's interested (though it's not very hard to implement).

@DanTup
Copy link
Member

DanTup commented Oct 18, 2021

By default, Dart-Code now uses LSP and not the original server protocol (if you set dart.previewLsp: false it will use the original protocol, but that will be removed in the future). This means the navigation needs to go via the server.

That said, I'm surprised what the server is doing is slow - can you reproduce this on a project you can share with an example navigation, as well as a log file (captured using Dart: Capture Analysis Server Logs so I can take a look? Thanks!

@DanTup DanTup added the awaiting info Requires more information from the customer to progress label Oct 18, 2021
@DanTup
Copy link
Member

DanTup commented Oct 18, 2021

jump-to-def/find-all-references

Also - can you clarify if both of these are slow, or just find-refs? My understanding is that the navigation event only provided the information required for Go-to-Definition, so I wouldn't expect to be able to improve Find-References with it - is that incorrect?

@JoeyScarr
Copy link
Contributor Author

Thanks for the quick response!

Also - can you clarify if both of these are slow, or just find-refs? My understanding is that the navigation event only provided the information required for Go-to-Definition, so I wouldn't expect to be able to improve Find-References with it - is that incorrect?

Ah, you're right. It's only Go to Definition that's improved by this.

By default, Dart-Code now uses LSP and not the original server protocol (if you set dart.previewLsp: false it will use the original protocol, but that will be removed in the future). This means the navigation needs to go via the server.

To provide some context, we're still stuck on Dart 1 for the short-medium term, hence we can't use the LSP yet.

I'll have to get back to you once I figure out how to provide a repro+trace that's both useful and shareable, but another piece of information that might be useful is we have a gencode library that has ~900 parts. It used to be one big file and we recently split it up -- performance was bad before but now it's even worse. It seems like the analysis server is constantly re-analyzing, even though nothing in the giant library is changing.

It's possible that the situation might improve if we split up the gencode into individual pieces that must be imported separately, and/or if we switch to Dart 2 and use newer versions of the analysis server, but both of those are a significant chunk of work. If you have any other ideas for short-term improvement, let me know.

FWIW, my suggested approach in the OP helps mitigate the situation somewhat, but it's still pretty bad as soon as one of the gencode files is opened; it seems that triggers the analyzer to keep analyzing forever.

@DanTup
Copy link
Member

DanTup commented Oct 26, 2021

To provide some context, we're still stuck on Dart 1 for the short-medium term, hence we can't use the LSP yet.

How long is that likely to be the case for? FWIW, it's likely non-LSP support will be dropped (and therefore Dart 1 support) from the extension in the future. It will still be an option to install older versions of the extension if you really need to use an old SDK, but straddling two language implementations is quite a burden that I'm keen to address. (The same will ultimately be true for debug adapters, as they're being migrated to the SDK and will need new-enough SDKs too).

It seems like the analysis server is constantly re-analyzing, even though nothing in the giant library is changing.

I'd usually ask for a log file, but if you're on Dart 1 it probably wouldn't be that useful because the code has changed so much, and if the issues did still exist on latest code and were fixed, you wouldn't get those fixes while using Dart 1.

I'm not against merging fixes to the non-LSP work if they're simple/reliable, although it sounds like it may not solve your entire issue. There have been many fixes to the analyzer since Dart 1 though, so it's possible the issues may just go away if you can upgrade.

Out of interest (because I ask this question a lot and nobody seems to answer) - what's the complicated part of moving to Dart 2? My understanding was that you could still run in non-null-safe mode, so you could use the newer SDKs/analyzer without needing to make the code null-safe (which I presume is usually the bulk of work). Is that not the case?

@JoeyScarr
Copy link
Contributor Author

How long is that likely to be the case for?

It's very hard to say, because we're currently blocked on figuring out a solution to the issues described below. I certainly understand it may not be practical/worthwhile to merge fixes to deprecated code that will soon be going away; as you say, if we end up stuck on Dart 1 for much longer it may be best to just stick to an old version of the plugin or my local fork of it.

Out of interest (because I ask this question a lot and nobody seems to answer) - what's the complicated part of moving to Dart 2? My understanding was that you could still run in non-null-safe mode, so you could use the newer SDKs/analyzer without needing to make the code null-safe (which I presume is usually the bulk of work). Is that not the case?

There are several blockers. Some of the lesser ones:

  • we have thousands of implicit casts that need to be removed, though IIRC early versions of dart 2 still support implicit casts so that's OK for now.
  • Null-safety is also something we can punt until later down the track.
  • There are potentially a lot of runtime type-related errors we'll need to flush out, but that's also tractable.

Our # 1 blocker by far, however, is solving the issue of dev cycle time. With dart 1 and dartium, the build time is pretty much instant. With dartdevc, our build takes several minutes to warm up on a cold build, and an incremental build takes 20-40 seconds. The incremental build is the biggest problem and we so far have no idea how to solve it. The only real avenue we can think of so far (short of hacking on the compiler) is to split up our code into multiple modules, but we haven't done any validation yet to see if that will actually help. If it does help, it'll be a huge migration effort.

If you have any sharable insights into dartdevc performance, how we might optimise it, and how long an incremental refresh cycle is expected to take with dartdevc on a large web app, that would be extremely helpful to us.

@DanTup
Copy link
Member

DanTup commented Oct 27, 2021

Ah, gotcha. Unfortunately I don't know how about the performance (or expected performance) or dartdevc, although if migrating to Dart 2 brings significant performance regressions in development it may be worth filing an issue at https://github.com/dart-lang/sdk for some discussion.

I'll leave it up to you if you want to file a PR for the changes described above - as long as the solution is reasonably clean I'm happy to include it if it will makes things easier. Removal of the non-LSP path isn't imminent, but it will happen at some point so I thought it worth mentioning.

@JoeyScarr
Copy link
Contributor Author

Sent a PR: #3649

Not sure if it's the cleanest solution but will leave up to your judgement whether it's mergeable or not.

@DanTup DanTup added this to the v3.29.0 milestone Nov 8, 2021
@DanTup DanTup added in editor Relates to code editing or language features is enhancement and removed awaiting info Requires more information from the customer to progress labels Nov 8, 2021
@DanTup
Copy link
Member

DanTup commented Nov 8, 2021

Fixed by #3649. Thanks!

@DanTup DanTup closed this as completed Nov 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in editor Relates to code editing or language features is enhancement
Projects
None yet
Development

No branches or pull requests

2 participants