apollo-client-devtools icon indicating copy to clipboard operation
apollo-client-devtools copied to clipboard

Extreme performance problems in version 4.14.0

Open dKustura opened this issue 1 year ago • 11 comments

I won't be able to give reproduction steps, profiler logs or any other useful information because this latest update caused such performance problems in my apps that the Chrome profiler can't even run. I will understand if you close this issue based on too little information but I still wanted to inform you that something is terribly broken in the latest update. Disabling the extension fixes the problem completely and the apps start running smoothly, just as they did 2 days ago.

Could be that there is a memory leak because all UI interactions get increasingly slower until the browser is completely unresponsive.

EDIT: There is definitely a memory leak, as the memory usage grows until the whole browser crashes.

  • OS: Windows 10
  • Browser: Google Chrome
  • Browser version: 125.0.6422.113
  • Extension version: 4.14.0

dKustura avatar May 30 '24 10:05 dKustura

Hey @dKustura 👋

Thanks for the report! I am so sorry to hear about the performance regression. I think the best course of action would be for us to roll back and investigate further.

We released 2 changes in 4.14. I have a suspicion which one it is, but in the absence of a reproduction, would you be willing to help me figure out which change it was?

https://github.com/apollographql/apollo-client-devtools/pull/1379 made a change to push events from the content script to the devtools rather than the devtools polling for it every 500ms. This seems less likely as this was designed to be a performance gain and uses a lot of the same mechanisms that were there before. This was merged in commit eacfbefe16021c0fa58e6df456405aba4c4f3fec.

https://github.com/apollographql/apollo-client-devtools/pull/1390 made a change to automatically reconnect ports when they disconnect to ensure the devtools continue to function after you've been idle for some time. This is my guess as to what is causing the spike in performance. This was merged in commit 3b0f0ea31fb1b9ed420b1730a233d9326fbcb7f3.

If you'd be willing to help me out, here is how you can build the devtools locally to help me figure out which change caused the regression:

  1. Clone the repo

  2. npm install

  3. npm run build -- --env TARGET=chrome This will create a build folder in the same directory that we'll use to install the extension locally.

  4. Visit chrome://extensions and turn on "Developer mode" (should be a switch on the upper-right corner) Screenshot 2024-05-30 at 9 31 32 AM

  5. Click "Load unpacked" and select the build folder from step 3 Screenshot 2024-05-30 at 9 32 31 AM

This should install the extension locally. You may want to disable the published extension while doing this, otherwise you'll have two "Apollo" tabs in the devtools and it may be difficult to tell them apart. You can then test this with your app. If you check out different commits to see which version causes the perf regression, rerun the build step first, then reload the extension by using the reload button on the extension on chrome://extensions: Screenshot 2024-05-30 at 9 36 23 AM

If you look at the git history, #1390 was merged after #1379. If you could first try checking out eacfbefe16021c0fa58e6df456405aba4c4f3fec (the commit that contains the push-based changes, but not the auto port reconnect), see if this first resolves the perf issue you're seeing. If that doesn't work, try checking out 0a5a26f7eab61e42a0b638e4b31169dbe84bb9f3 (the commit before the push-based changes).

This would greatly help me nail down where to look. Unfortunately our development app is pretty basic so its a bit more difficult to test with a full product with a large schema. In the mean time, I'll get a release out that reverts these changes so that the extension can at least become reusable again. Thanks!

jerelmiller avatar May 30 '24 15:05 jerelmiller

I just released v4.14.1 that reverts the latest changes. This should hit the stores within the next couple hours. Thanks again for reporting the issue and apologies for introducing it in the first place!

jerelmiller avatar May 30 '24 16:05 jerelmiller

Hey @jerelmiller, thank you for the quick response and update!

Also, thank you for the detailed steps describing how to debug this. Here's my feedback:

First I followed your advice of checking out out https://github.com/apollographql/apollo-client-devtools/commit/eacfbefe16021c0fa58e6df456405aba4c4f3fec, the perf issue remained.

Then I checked out https://github.com/apollographql/apollo-client-devtools/commit/0a5a26f7eab61e42a0b638e4b31169dbe84bb9f3 which fixed the perf issue.

To be extra sure, I rebased the main branch onto itself to remove the https://github.com/apollographql/apollo-client-devtools/commit/eacfbefe16021c0fa58e6df456405aba4c4f3fec commit. So the state I had on my branch at this point was (from oldest to newest commit):

  • https://github.com/apollographql/apollo-client-devtools/commit/0a5a26f7eab61e42a0b638e4b31169dbe84bb9f3 chore(deps): update all dependencies - patch updates
  • Automatically reconnect the ports when they disconnect (#1390)
  • Version Packages (#1388)

So I'd definitely say the problem is in the push-based changes.

dKustura avatar May 30 '24 16:05 dKustura

How interesting! Funny how perception is different than reality. I'm actually a bit glad to hear the port reconnect is not the issue as I believe that change fixes the cause of most of the issues we see with the devtools becoming unresponsive after a period of idle time. The push-based changes are more of a nice-to-have anyways.

With this info, I'll get a new PR out to reintroduce the port reconnect in the next couple days so we can at least have these changes. Thank you so much for taking the time to help me out here!

jerelmiller avatar May 30 '24 16:05 jerelmiller

No problem, glad to help 🙂

dKustura avatar May 30 '24 16:05 dKustura

@jerelmiller We just ran into this issue in our prod today (oh those unconditional connectToDevTools: true), and some of our devs saying they still see the issues even on 4.14.1 version 🤔 I haven't got the update in my Chrome yet to verify it resolves it.

kliakhovskii-brex avatar May 30 '24 20:05 kliakhovskii-brex

Thats odd. The version before 4.14.0 (4.13.1) was out for at least a week. The patch version 4.14.1 just reverted all the changes from 4.14.0 🤔.

If you want to force it to upgrade, you should be able to do it by enabling "Developer mode" on chrome://extensions, then clicking the "Details" button for the "Apollo DevTools" extension, then clicking the "Upgrade" button.

I'm really curious if you're seeing the issue persist in the patch version as well. So very sorry about this!

jerelmiller avatar May 30 '24 20:05 jerelmiller

Just updated to 4.14.1 and it helped for me, so likely it resolves the issue. Thx!

PS I wasn't really aware that this thing runs even if Dev Tools are closed :) I had impression that it runs only when dev tools are actually in use.

kliakhovskii-brex avatar May 30 '24 21:05 kliakhovskii-brex

@kliakhovskii-brex glad to hear the upgrade solves it. Thanks for confirming.

I wasn't really aware that this thing runs even if Dev Tools are closed

It sorta half does and half doesn't. The devtools scripts themselves won't run until you open devtools, but in order to get access to your client, we have a content script that injects a small runtime script on your page that has access to window.__APOLLO_CLIENT__, and this will send messages to the devtools scripts. So you will have something running on your page even without devtools open, but only part of the extension.

As I'm typing this, I think I realized my mistake and why the push-based update behaved poorly and it has to do with the fact that I was sending the operations to the devtools scripts event if devtools weren't open. I think I have some ideas on how to improve this to avoid the perf issue I introduced.

At this point I'm just thinking out loud 😆. Thanks for prompting some thinking on my end!

jerelmiller avatar May 30 '24 22:05 jerelmiller

No worries, glad you rolled it back so quickly! Thx!

kliakhovskii-brex avatar May 30 '24 23:05 kliakhovskii-brex

FYI I just re-released the changes to automatically reconnect the ports when they disconnect in 4.14.2. Please do let me know if you see any regressions from this change!

jerelmiller avatar Jun 04 '24 22:06 jerelmiller

Hey all 👋

I haven't seen any activity on this issue for some time, so I'll assume this issue been solved at this point.

FYI, I have a PR open that will allow you to inspect multiple clients. In this PR is an optimization that will only sync the data needed for the view you're inspecting. Currently all data is synced every 500ms, which means we are syncing all query, mutation, and cache data even if you're only inspecting the "Queries" tab. I'm hoping this will also further enhance some of the performance with the devtools for larger apps.

Let me know if you see anything else and I'll be glad to reopen this, but for now I'll go ahead and consider this issue resolved 🙂.

jerelmiller avatar Jul 12 '24 00:07 jerelmiller

Do you have any feedback for the maintainers? Please tell us by taking a one-minute survey. Your responses will help us understand Apollo Client usage and allow us to serve you better.

github-actions[bot] avatar Jul 12 '24 00:07 github-actions[bot]