dart-lang / webdev

A CLI for Dart web development.
https://pub.dev/packages/webdev
212 stars 75 forks source link

Debug stepping/pausing with Dart Debug extensions is very slow with remote servers #975

Open DanTup opened 4 years ago

DanTup commented 4 years ago

When running on GitPod, it takes around 10 seconds for a breakpoint to be hit. I previously assumed this was the Chrome issue but if I open the inspector for the extension, I see around 138 (sequential) requests when a breakpoint was hit, ranging from 25ms to over 2s adding up to around 10s.

Screenshot 2020-04-29 at 14 13 42

We're forcing WebSockets for the VM Service proxy from VS Code, but I don't think they're currently supported for the debug extension backend - I wonder if that would make a significant difference here?

grouma commented 4 years ago

This is somewhat related to the Chrome issue. When we hit a breakpoint, or pause execution for that matter, we must translate each JS frame to its corresponding Dart context. A deeply nested stack (which is part of the Chrome issue) will cause a significant number of requests to the extension to execute evaluations. The extension does support basic batching but not for these type of calls. That's something we could look into but not in the near future. I'm also curious if we can get the required information from the compiler directly.

cc @annagrin

annagrin commented 4 years ago

I'm also curious if we can get the required information from the compiler directly.

I suspect that frontend server, compiler, dwds, and interactions between them could be improved to reduce the amount of information we transfer between various services. Working on collecting the issues at the moment, will add the excessive amount of chrome calls to it as well, thanks!

devoncarew commented 4 years ago

@DanTup - can you re-test this? One round of performance improvements have landed in flutter/flutter master.

DanTup commented 4 years ago

@devoncarew this still looks slow. I tested with a slightly older version and it was 28s, then with master and it was around 18s.

Screenshot 2020-06-18 at 09 16 44 Screenshot 2020-06-18 at 09 16 18

gitpod_20s_requests.har.zip

The version was:

[✓] Flutter (Channel master, 1.20.0-1.0.pre.91, on Linux, locale en_US)
    • Flutter version 1.20.0-1.0.pre.91 at /home/gitpod/flutter
    • Framework revision 9c3f0faa6d (10 hours ago), 2020-06-17 18:47:54 -0400
    • Engine revision 965fbbed17
    • Dart version 2.9.0 (build 2.9.0-14.0.dev 2b917f5b6a)
DanTup commented 4 years ago

Actually, the time in those screenshots looks questionable.. When I open the har file directly, I see all the same requests, but it now finishes after around 5.6s! 🤷‍♂️

Edit: The 6 seconds (as shown in the har above) seems accurate from timing manually.

DanTup commented 4 years ago

I know it might not work everywhere, but I rigged a fork to use WebSockets instead of SSE for the debug extension backend and tested it on GitPod. The difference in performance is significant - with the counter app I see below 1s between clicking the button and the GitPod debugger UI pausing (the timings noted above were around 6 seconds).

I think it's a combination of requests being able to overlap and the reduced per-message latency With SSE it seems the penalty is quite high - the messages are being sent sequentially, but also the round-trip time is quite high (likely a combination of the per-HTTP-request overhead and the longer latency for something like GitPod):

Screenshot 2020-06-23 at 09 06 29

The code isn't production-ready, but if it's reasonable to support both WS and SSE here (as we do with the VM service proxy) I can tidy it up and open a PR to iterate on.

devoncarew commented 4 years ago

FYI @annagrin and @grouma (Gary is OOO right now)

I think supporting both would be valuable; we'll also want to look at performance improvements to package:sse as well, in order to speed up the cases where we do need to use SSE.

annagrin commented 4 years ago

I will defer the WS vs SSE issue to @grouma as he has much more context there than I do. I am currently working on storing compiler metadata and reading it in dwds instead of getting source information from chrome. This should reduce the amount of backend calls we make per breakpoint overall. If you are interested in trying it i can send links to flutter and dwds PRs when ready.

DanTup commented 4 years ago

I tried a few other things - like batching SSE requests between client and server (eg. if the client queues multiple requests while the first is in-flight, the rest can be batched in one HTTP POST). It seemed to work well in the tests, but made no difference on GitPod (perhaps because the requests are server-to-client, and being sent only one-at-a-time?).

If @grouma is happy to support both, I'll tidy up my code and open a PR.

If you are interested in trying it i can send links to flutter and dwds PRs when ready.

Sure! I'm able to run branches of flutter/dwds/sse on GitPod relatively easily, so if you have changes worth trying out, I can do some testing without them needing to have landed in Flutter (or even dwds/sse). Thanks!

grouma commented 4 years ago

I tried a few other things - like batching SSE requests between client and server (eg. if the client queues multiple requests while the first is in-flight, the rest can be batched in one HTTP POST). It seemed to work well in the tests, but made no difference on GitPod (perhaps because the requests are server-to-client, and being sent only one-at-a-time?).

We batch scripted parsed events through the Dart Debug Extension. These events were (and still are) on the critical path for IPL. We haven't extended the batching support further as initial testing didn't prove that helpful.

If @grouma is happy to support both, I'll tidy up my code and open a PR.

We'll need to continue support of SSE for internal users but I'm happy to review support for websockets. We'll probably want to embed the configuration in the injector and have the extension adjust accordingly.

DanTup commented 4 years ago

I was using the scheme on the URI in the extension to decide which protocol to use (that way the configuration was only required on the server side). I only implemented it for the debug extension for now (not the injected client, since I figured that had much less traffic and was relying on the keepAlive behaviour for refreshes, which I think the extension doesn't need).

I'll open a PR with what I have so far anyway, and we can iterate there. Thanks!