[🐞] 1.8.0 regressions (slow service worker and larger app)

tuurbo commented 1 month ago

Which component is affected?

Qwik Runtime

Describe the bug

I tried to update to qwik from 1.7.2 to 1.8.0 today (no other deps were updated).

TLDR: In my opinion it seems like the service worker is loading too much unrelated code on some interactions.

I'm getting up to 2.5 second delays on chrome 127 (6 seconds on firefox 127) for files to load from the service worker when interacting with an "add to cart" button on the page. If the service worker is enabled and i click the button, 132 requests and 103kb is loaded in the network tab. If I disable the service worker completely and click the button, only 44 requests and 67kb is loaded in the network tab and my app interactions go back to being instant.
File size of my dist/build folder went from 1.09MB / 483 files to 1.25MB / 662 files. I haven't found any server code leaking to the browser but its possible I'm missing it.

43 of my 101 e2e tests are no longer passing. Seems related to the much slower service worker because playwright is timing out after 5 seconds while trying to find elements after interacting.

Service worker usage on the chrome storage tab went from ~226kb on first page load to ~600kb. Maybe irrelevant.

I tried deleting my node_modules folder and used pnpm i --force.

I tried setting moduleResolution to Bundler.

I also reverted back to qwik 1.7.2 and my app went back to working as it expected.

Not sure how i can create a reproduction of this problem because its only an issue with some interactions in my ecommerce website, not all. So I'm hoping someone else might be experiencing this issue or someone can confirm its a 1.8.0 regression.

Reproduction

none yet

Steps to reproduce

No response

System Info

System:
    OS: Windows 10 10.0.19045
    CPU: (20) x64 12th Gen Intel(R) Core(TM) i9-12900HK
    Memory: 10.85 GB / 31.68 GB
  Binaries:
    Node: 20.14.0 - C:\Program Files\nodejs\node.EXE
    npm: 10.7.0 - C:\Program Files\nodejs\npm.CMD
    pnpm: 9.5.0 - C:\Program Files\nodejs\pnpm.CMD
    bun: 1.1.23 - ~\.bun\bin\bun.EXE
  Browsers:
    Edge: Chromium (127.0.2651.74)
    Internet Explorer: 11.0.19041.4355
  npmPackages:
    @builder.io/partytown: ^0.10.2 => 0.10.2
    @builder.io/qwik: 1.8.0 => 1.8.0
    @builder.io/qwik-city: 1.8.0 => 1.8.0
    typescript: 5.4.5 => 5.4.5
    vite: 5.3.5 => 5.3.5

Additional Information

No response

gioboa commented 1 month ago

Thanks for your report @tuurbo Are you using ServiceWorkerRegister component?

tuurbo commented 1 month ago

@gioboa Yes <ServiceWorkerRegister /> and no modification, just the default that comes with qwik.

gioboa commented 1 month ago

I'm wondering if a standard application with few routes can simulate the problem. Without a minimum reproduction it's really hard to figure out the problem. 😅

gioboa commented 1 month ago

if I disable the service worker completely and click the button, only 44 requests and 67kb is loaded in the network tab and my app interactions go back to being instant.

This I interesting 🤔 Have you done a check with the SW to figure out the content of the extra chunks?

tuurbo commented 1 month ago

Yeah I tried to build an example repo but couldn't produce similar results. I'm assuming the larger scale of my app is a factor.

tuurbo commented 1 month ago

if I disable the service worker completely and click the button, only 44 requests and 67kb is loaded in the network tab and my app interactions go back to being instant.

This I interesting 🤔 Have you done a check with the SW to figure out the content of the extra chunks?

I noticed some pages were being loaded like the user account address page and a checkout page. Both should have no strong relation to the product page the "add to cart" button was clicked on.

wmertens commented 1 month ago

The increased amount of files coupled with the modest increase in size is due to 1.8 leaving the qrl segment bunching up to the bundler. Rollup could do better, see https://github.com/rollup/rollup/issues/5574 . Hopefully we'll come up with optimizations for it, but it shouldn't really matter much.

The service worker downloading more is concerning though. The q-manifest is now being created more correctly (some of the filenames were wrong), and maybe that causes a bigger import graph. Maybe there always was a bug and the wrong filenames prevented the issue?

tuurbo commented 1 month ago

The service worker downloading more is concerning though. The q-manifest is now being created more correctly (some of the filenames were wrong), and maybe that causes a bigger import graph. Maybe there always was a bug and the wrong filenames prevented the issue?

A bug on my end or Qwiks?

tuurbo commented 1 month ago

After more testing I can confirm that the service worker is loading multiple routes/components from other pages (account, cart, checkout pages) that are unrelated to the product page that the "add to cart" button interaction is happening on. This was not happening with 1.7.2. I'm assuming what is making it worse is some of those unrelated files are being loaded before the relevant files needed for the button interaction.

wmertens commented 4 weeks ago

Thanks for investigating. Here's what I think is happening: because now the manifest correctly names files, the route map is now part of the import graph, and that makes it import all routes

devcaeg commented 3 weeks ago

I want to report that I have the same problem. But it is not always the same, there are times when I do a reload on the web, and more than 1000 “q” files are downloaded, other times only 40 “q” files are downloaded, and other times around 500 “q” files.

The correct number is when approximately 40 “q” files are downloaded.

ianlet commented 3 weeks ago

Seems like I'm having the same issue: https://github.com/QwikDev/qwik/issues/6815

I'm working on a minimal reproduction (simulating many pages + many possible interactions in the page + reasonably large modules). I'll keep you updated if I have something.

Worst case, I could open source my app (I'm planning to do it anyway), but it will not be a minimal reproduction of the issue :sweat_smile:

ianlet commented 3 weeks ago

I think @wmertens is pointing into the right direction: not all the routes are being prefetched, but still way too many app modules are being walked on to prepare which one should be prefetched.

I debugged the service worker and it is "choking" on this recursion for a while before actually prefetching the app modules, as illustrated in this profiling (see the recursive forEach in the DOM Workers under the track localhost (3/3) - PID: 13380).

In my case, I have 596 app bundles in total. On the very first prefetch, this function is trying to load 17 app bundles. Just for this single call (there are at least dozens of them), it walked on 3,611,170 bundles and (best case) queued 63 of them to be prefetched.

I haven't been able to create a repository to reproduce the issue in isolation yet because it occurs when the app becomes complex enough (not necessarily in terms of number of pages, but in terms of complexity of each page/module). So instead, see the files attached to grab the inputs of what jammed the service worker when calling the prefetchBundleNames function.

File:qwik-noma-appBundles.json (1st arg of the prefetchBundleNames function) File:qwik-noma-prefetchAppBundleNames.json (5th arg of the prefetchBundleNames function)

With ^this, you should be able to reproduce an actual test case.

Extra data loaded by the service worker: File:qwik-noma-linkBundles.json File:qwik-noma-libraryBundleIds.json

ianlet commented 3 weeks ago

As I don't know the internals of the project well enough, I haven't thought about the bigger picture to know if the root cause was a larger design issue.

But, if we simply want to prevent the service worker to walk unnecessarily over all the bundles multiple times, tracking which bundles were already walked does the trick.

This patch fixes the issue:

diff --git a/packages/qwik-city/src/runtime/src/service-worker/prefetch.ts b/packages/qwik-city/src/runtime/src/service-worker/prefetch.ts
index 5ecd7b670..358c8fc14 100644
--- a/packages/qwik-city/src/runtime/src/service-worker/prefetch.ts
+++ b/packages/qwik-city/src/runtime/src/service-worker/prefetch.ts
@@ -1,7 +1,11 @@
-import type { AppBundle, Fetch, LinkBundle } from './types';
+import type { AppBundle, Fetch, LinkBundle, VisitedBundles } from './types';
 import { awaitingRequests, existingPrefetchUrls, prefetchQueue } from './constants';
 import { cachedFetch } from './cached-fetch';
-import { getAppBundleByName, getAppBundlesNamesFromIds } from './utils';
+import {
+  getAppBundleByName,
+  getAppBundlesNamesFromIds,
+  getUnvisitedAppBundlesNames,
+} from './utils';

 export const prefetchBundleNames = (
   appBundles: AppBundle[],
@@ -28,12 +32,19 @@ export const prefetchBundleNames = (
     }
   };

+  const visitedAppBundles: VisitedBundles = {};
+
   const prefetchAppBundle = (prefetchAppBundleName: string | null) => {
+    if (!prefetchAppBundleName) {
+      return;
+    }
+
+    visitedAppBundles[prefetchAppBundleName] = true;
+
     try {
       const appBundle = getAppBundleByName(appBundles, prefetchAppBundleName);

       if (appBundle) {
-        const importedBundleNames = getAppBundlesNamesFromIds(appBundles, appBundle[1]);
         const url = new URL(prefetchAppBundleName!, baseUrl).href;
         const queueIndex = prefetchQueue.indexOf(url);

@@ -54,6 +65,11 @@ export const prefetchBundleNames = (
           }
         }

+        const importedBundleNames = getUnvisitedAppBundlesNames(
+          appBundles,
+          visitedAppBundles,
+          appBundle[1]
+        );
         importedBundleNames.forEach(prefetchAppBundle);
       }
     } catch (e) {
diff --git a/packages/qwik-city/src/runtime/src/service-worker/types.ts b/packages/qwik-city/src/runtime/src/service-worker/types.ts
index e3969942a..352fb1a1a 100644
--- a/packages/qwik-city/src/runtime/src/service-worker/types.ts
+++ b/packages/qwik-city/src/runtime/src/service-worker/types.ts
@@ -22,6 +22,10 @@ export type AppBundle =

 export type LinkBundle = [routePattern: RegExp, bundleIds: number[]];

+export interface VisitedBundles {
+  [key: string]: boolean;
+}
+
 export type Fetch = (r: Request) => Promise<Response>;

 export type AwaitingRequests = Map<
diff --git a/packages/qwik-city/src/runtime/src/service-worker/utils.ts b/packages/qwik-city/src/runtime/src/service-worker/utils.ts
index 80388d094..d30822e0d 100644
--- a/packages/qwik-city/src/runtime/src/service-worker/utils.ts
+++ b/packages/qwik-city/src/runtime/src/service-worker/utils.ts
@@ -1,4 +1,4 @@
-import type { AppBundle, AppSymbols } from './types';
+import type { AppBundle, AppSymbols, VisitedBundles } from './types';

 export const getCacheToDelete = (appBundles: AppBundle[], cachedUrls: string[]) =>
   cachedUrls.filter((url) => !appBundles.some((appBundle) => url.endsWith(appBundle[0])));
@@ -35,3 +35,13 @@ export const computeAppSymbols = (appBundles: AppBundle[]): AppSymbols => {
   }
   return appSymbols;
 };
+
+export const getUnvisitedAppBundlesNames = (
+  appBundles: AppBundle[],
+  visitedAppBundles: VisitedBundles,
+  bundleIds: number[]
+) => {
+  return getAppBundlesNamesFromIds(appBundles, bundleIds)
+    .filter((b) => b) // Could be removed if getAppBundlesNamesFromIds doesn't return null values
+    .filter((b) => !visitedAppBundles[b!]);
+};

I can open a PR with the fix + a regression test if you want.

ianlet commented 2 weeks ago

For those encountering this issue when upgrading to qwik@1.8:

When you initially created your qwik project with qwik-city, the starter template configured your project to use a Service Worker (SW) to prefetch modules in background to provide a more snappy experience for your users. Unfortunately, qwik@1.8.0 revealed a bug in that prefetching strategy (that was already there) making now the whole experience very laggy or even freezing for larger projects.

As a temporary workaround, you have 2 options:

Downgrade to qwik@1.7.3 to keep using the Service Worker as is

Remove the Service Worker to switch to a new prefetching strategy. See the patch below for an example of how to configure it:


diff --git a/package.json b/package.json
--- a/package.json
+++ b/package.json
@@ -29,8 +29,8 @@
"devDependencies": {
 "@auth/core": "^0.34.2",
 "@auth/qwik": "0.2.2",
-    "@builder.io/qwik": "1.7.3",
-    "@builder.io/qwik-city": "1.7.3",
+    "@builder.io/qwik": "1.8.0",
+    "@builder.io/qwik-city": "1.8.0",
 "@deno/kv": "^0.8.1",
 "@modular-forms/qwik": "^0.26.1",
 "@types/eslint": "^8.56.11",
diff --git a/src/entry.ssr.tsx b/src/entry.ssr.tsx
--- a/src/entry.ssr.tsx
+++ b/src/entry.ssr.tsx
@@ -26,5 +26,11 @@ export default function (opts: RenderToStreamOptions) {
   lang: "en-us",
   ...opts.containerAttributes,
 },
+    prefetchStrategy: {
+      implementation: {
+        linkInsert: "html-append",
+        linkRel: "modulepreload",
+      },
+    },
});
}
diff --git a/src/root.tsx b/src/root.tsx
--- a/src/root.tsx
+++ b/src/root.tsx
@@ -1,9 +1,5 @@
import { component$ } from "@builder.io/qwik";
-import {
-  QwikCityProvider,
-  RouterOutlet,
-  ServiceWorkerRegister,
-} from "@builder.io/qwik-city";
+import { QwikCityProvider, RouterOutlet } from "@builder.io/qwik-city";
import { RouterHead } from "./components/router-head/router-head";
import { PageViewTracker } from "~/components/analytics/page-view-tracker";

@@ -35,7 +31,6 @@ export default component$(() => { />

     <RouterHead />

Note: if you decide to use the new prefetching strategy, be aware that your FCP and LCP scores on Lighthouse (PageSpeed insights) might be impacted negatively, even though your user experience (interactivity with your app) might not be.

shairez commented 1 week ago

Thanks a lot @ianlet for the research!

We're working on a fix for this If you can, please DM me on discord if you're on our server Thanks!

shairez commented 1 week ago

Hey good people! @ianlet @devcaeg @tuurbo

it would be great if you could test if my PR fixes your issues.

You can install it via the pkg.pr.new urls here - https://github.com/QwikDev/qwik/pull/6863#issuecomment-2334954113

As soon as you give me the 👍 we could merge this fix

Thanks!

tuurbo commented 6 days ago

The page interactions are no longer delayed for 2 to 3 secs, so that part seems to be fixed.

But unrelated code is still loaded on some interactions. For example, I clicked an "add to cart" button and 5 unrelated routes were loaded as well. On 1.7.1, 41kbs of js were loaded on that button click. On the PR fix 75kbs of js are loaded.

shairez commented 6 days ago

thanks @tuurbo !

Is there a way for you to create a repro with multiple routes similar to your app?

I saw that you tried in the past, but maybe adding a few more routes will help?

tuurbo commented 6 days ago

I gave it another shot but couldn't reproduce the issue by creating a new multiple page repo :/

shairez commented 5 days ago

Thanks @tuurbo !

I tried it as well with this repo by switching it to the old SW but no luck.

I think it's best to merge my PR as it seems to solve part of the issue, and continue investigating, hopefully someone will be able to provide a good repro (maybe even @ianlet 😊 )

QwikDev / qwik