nuxt-community / pwa-module

Zero config PWA solution for Nuxt.js
https://pwa.nuxtjs.org
MIT License
1.24k stars 171 forks source link

Icon resize task blocks the main thread during build #171

Closed jhnns closed 5 years ago

jhnns commented 5 years ago

Version

2.6.0

Reproduction link

https://github.com/jhnns/nuxt-pwa-build-perf-demo

Steps to reproduce

What is expected ?

The icon resize task should start as soon as possible in a different process/thread to speed up the build.

What is actually happening?

Here is a flamegraph that demonstrates that the icon resize task takes up more than half of the build time. But the bigger problem is that the task blocks the main thread which means that webpack can't start to do its work in the mean time. Here I've marked the resize task with red: Bildschirmfoto 2019-03-30 um 17.57.58 Kopie.jpg

You can see that webpack starts its work only after around 16s.

Additional comments?

Spinning up a separate process doesn't come for free. Usually it's better to have a separate thread for this kind of task. We could do this with Node.js worker threads (https://nodejs.org/api/worker_threads.html) which are only available for Node >10.

This bug report is available on Nuxt community (#c116)
pi0 commented 5 years ago

Hi @jhnns. Thanks for reporting this issue. Actually, this problem was not visible because normally the original icon is small-sized.

I've tried to make the issue more visible by using a bigger icon in the test fixture:

image

I'm working on an icon module rewrite in a way that resize microtasks running in the background. (Initial read may be still required before build to generate MD5 hash that is used in filenames)

pi0 commented 5 years ago

Implemented in a4a457efe98461cf28249742952ca49e9ebfe847 (with some enhancenments including cache in later commits) I used normal cp.fork to support node <10.

image

jhnns commented 5 years ago

this problem was not visible because normally the original icon is small-sized

I never changed the icon. I just used the default icon (see also reproduction link).

Implemented in a4a457e (with some enhancenments including cache in later commits) I used normal cp.fork to support node <10.

Awesome, the flame graph looks good 👍. I just left some comments at the commit :)

pi0 commented 5 years ago

Thanks, @jhnns for the comments.

pi0 commented 5 years ago

Released in 3.0.0-beta.15