-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
Improve completion time for large code bases #48788
Comments
Some motivating user-filed bugs relating to this: |
I profiled
Those numbers look great. So, we need more investigation to find if there are reproducible situations which lead to much higher completion times. Perhaps more symbols. Perhaps add some 'changedFile' events preceding the complete request. Perhaps Windows-specific, where stdin/stdout might be slower... |
This adds a section to the output like this: ``` ### Percentile metrics p50 p90 p95 count > 2s ms per completion 4 6 7 0 ``` Bug: #48788 Change-Id: I868580324b3bc83605aa6466ffd5799625d1a9f3 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/240941 Commit-Queue: Samuel Rawlins <[email protected]> Reviewed-by: Brian Wilkerson <[email protected]>
When I look at flutter_gallery, I see these as the slowest completions, when erasing a whole identifier/keyword and complete where it was:
Notes:
|
I wouldn't expect that to be the case because if we're removing part of the file before completing then we're having to analyze the whole file before every completion. But I'm not an expert in performance questions.
That's definitely interesting. Can we tell what the performance is if completion is happening between complete members? I'm wondering whether it's because we're at the level of a class member or whether the AST we're seeing after recovering from the error is causing the problem. If it's the former, then that's an opportunity; if it's the latter, then I don't expect that users are in this situation very often.
The return type is optional for methods, setters, and getters, so no recovery is required. (Although I would expect recovery if the type were nullable and the question mark isn't being deleted.)
That case might well recover better because the presence of the initializer seems like a clear signal that we have a field declaration. |
These were done with the default overlay, none, so no overlays are created, and the code should always be error-free. |
When I use the
|
I am experiencing very high latency in our code base. Things like >60000 ms. There is no task (being reported in the Analysis Server Diagnostics) that is taking this 60000ms, so it is something happening under the hood that I cannot see through the diagnostics page. It is happening very often, but it is specially bad when we add or remove a dependency in a deeper level of the contexts being analyzed, for example. The code completion time is also affected by slowness (not latency this time). Let me know what kind of information I can provide to help investigating that. |
Great data, @gabrielgarciagava ! Could you also report:
|
@gabrielgarciagava additionally to what @srawlins have said. If you are running on new enough analyzer (e.g. Flutter master will certainly have it) then there should be "Collect Report" option at the top of the "Analysis Server Diagnostics" page. |
Shouldn't |
TIL 🎉 I love it. Thanks @incendial |
134
No plugins.
Output of
VSCode
7601 |
I am afraid I cannot handle this file. At least not before checking with the legal parts of the company. I see there are package names there, so I need to take care what can be shared and what cannot. |
CC @bwilkerson @scheglov I find this data very interesting:
If, broadly speaking, we find that multiple contexts can linearly slow down completion (and other responses, as seen above), that would also boost the priority of improving the multi-context situation. |
Sobre extra context, in case it helps. For the 8.5GB, I had started the IDE and added a new dependency in one of the contexts that impacts a lot of other contexts. No more than that. Just saying that the IDE was not running for so long, not sure if it has any impact in the memory. Well, you can see that in the elapsed time. Second, for the high number of contexts, we do divide our code in a lot of small packages within the same repository. |
Ah, it was an oversight to include context names which might be project specific. You can scrape this information by running the following script (it will also scrape lint and plugin names for good measure): import 'dart:convert';
import 'dart:io';
// Usage: dart scrape.dart report.json
void main(List<String> args) {
final data = jsonDecode(File(args.first).readAsStringSync());
for (var entry in data['contexts']) {
entry['name'] = '@@@';
entry['lints'] = entry['lints'].length;
entry['plugins'] = entry['plugins'].length;
}
File(args.first).writeAsStringSync(JsonEncoder.withIndent(' ').convert(data));
} The rest of the report just contains information about the analyser state itself and should not contain any user data. |
@mraleph Thanks for the script. Looks good to me now. Here it is: |
Thanks! Initial glance over the report:
It seems to me that in this case at least, the memory usage has successfully gone down again. These numbers seem rather similar to what I see when loading https://github.com/kodecocodes/rwf-materials.git from cache. |
Thanks for checking the data. Please let me know if I can help providing more data to you. Those slow times are being a hassle for the day to day work, but I'm also not sure if this issue has high priority for you guys or not. It looks like not so many people are currently affected by this problem, right? |
An update on the report: it has 3,277,209 |
We don't have anything running continually like you said. However, we do have one script that runs pub get on a multiple packages. For this 15min experiment I did run it once. In this experiment, I did modify the pubspec of one deep down dependency. I think around 50 packages were updated and thus had run pub get. |
That's probably enough --- if 50 packages were updated with enough time between each update (say a few seconds which doesn't seem unlikely) that would probably do it. |
Cool. Should I try to create a "small" project sample with multiple packages that uses this kind of script to share with you? |
I don't think that's currently necessary, thanks though. |
Does this leak affect performance as well? |
Unfortunately I don't think that's the cause of the 60+ seconds latency you reported above, no. |
(though, with a big enough leak we might get in trouble with the amount of memory available on the system, leading to swapping etc which certainly would impact performance). |
Ok. I was wondering if all those In any case, after your fix is released, I will do another run with the analyzer diagnostics and post it here. |
…fterContextsCreated` is called If again and again editing, say, a `pubspec.yaml` with proper timing there is a "temporary leak" that repairs itself once the analyzer finishes (some time after the editing stops). What happens is that old contexts are saved in the `declarationsTracker` but eventually cleared in the `afterContextsCreated` call. In a test on Windows (on Linux I'd currently run into https://dartbug.com/52703) I opened the entire "pkg" folder and edited a `pubspec.yaml` file every 5 seconds. The analyzer went from using something along the lines of 700MB of heap to using around 2.5 GB of heap after 25 edits and 17GB (!) of heap shortly before stopping at 250 `pubspec.yaml` edits. After the editing stopped (and clicking the GC button in observatory) the heap usage went down to ~650MB heap used. This fixes the problem by clearing the `declarationsTracker` on the `afterContextsDestroyed` call too. In the same test it stays at around 300-700MB of heap. Possibly related to: #48788 #52447 Change-Id: Ia38cc946a1f36fa8c5b6804f79cbc8dd96c21030 Reviewed-on: https://dart-review.googlesource.com/c/sdk/+/309722 Reviewed-by: Brian Wilkerson <[email protected]> Reviewed-by: Konstantin Shcheglov <[email protected]> Commit-Queue: Jens Johansen <[email protected]>
@gabrielgarciagava what is your current experience with analyzer latency on your code base? |
Still quite bad. Codegens, changing branch, updating dependencies, all of these degrades de performance for the next couple of minutes, for example. I am following a bit the progress of "workspaces". I even gave it a try on the master branch 1 or 2 weeks ago. I did some codegens (which is something that currently degrades a lot the performance for a couple of minutes), and the analyzer seems to respond very quickly after that. |
@gabrielgarciagava how big is your code base? do you use code generation? how much generated code is there? (and what type of code generators do you use?) |
We have something around 120 packages. I have no idea about the LOC. We do use code generation quite a lot, like json_serializable and functional_data (which generates ==, hashCode, copyWith, etc). I also don't know the number of generated lines of code. What type of data exactly are you looking for? I can try to calculate it. |
Large code bases appear to strongly negatively affect code completion time. To be more precise, code completion in libraries with a large namespace, or a large potential namespace (suggesting things that can then be auto-imported) may be much slower than smaller code, even when accounting for the "new protocol" being added to IntelliJ, and which may be used in LSC clients, like VS Code, already.
This task is to measure it, improve it, and decide on a good exit criteria. There are, of course, always ways to improve performance; for this task we must choose a meaningful exit criteria so as to give the task a meaningful priority.
The text was updated successfully, but these errors were encountered: