Skip to content
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

perf: allow using @parcel/watcher for dev watcher #20179

Merged
merged 6 commits into from
Apr 19, 2023
Merged

Conversation

danielroe
Copy link
Member

@danielroe danielroe commented Apr 10, 2023

❓ Type of change

  • πŸ“– Documentation (updates to the documentation, readme or JSdoc annotations)
  • 🐞 Bug fix (a non-breaking change that fixes an issue)
  • πŸ‘Œ Enhancement (improving an existing functionality like performance)
  • ✨ New feature (a non-breaking change that adds functionality)
  • 🧹 Chore (updates to the build process or auxiliary tools and libraries)
  • ⚠️ Breaking change (fix or feature that would cause existing functionality to change)

πŸ“š Description

This is an experimental PR swapping out chokidar for @parcel/watcher for testing on Windows and other OSes for performance improvements. See #20176 for some context.

@parcel/watcher supports Watchman for significant perf improvements when watching large file trees and is used under the hood by VSCode.

πŸ“ Checklist

  • I have linked an issue or discussion.
  • I have updated the documentation accordingly.

@pi0
Copy link
Member

pi0 commented Apr 10, 2023

Nice idea trying this. Some notes

Checking pkg contents it seems being shipped with whole binaries in the prebuilds dependency so probably safer if we avoid migrating for main cli package (at least here and instead try on nuxi-edge. (there is also a gyp file in the package not sure why!). Chokiar in contrast only has optional fsevents dependency which is common enough and already a dep of cli (also i guess vite)

It seems only ships with a CJS build, no exports field. ESM support is via CJS. And src dir is shipped without any reason.

fsevents used in other packages including rollup and nitro is also already there so it might be worth more if being a synced change move.

@stafyniaksacha
Copy link
Contributor

stafyniaksacha commented Apr 10, 2023

(there is also a gyp file in the package not sure why!).
And src dir is shipped without any reason.

This is to allows to build from source if prebuilds does not match OS arch I guess

@pi0
Copy link
Member

pi0 commented Apr 10, 2023

Yes, and that's not exactly the best way to distribute a binary...

@stafyniaksacha
Copy link
Contributor

Yes, and that's not exactly the best way to distribute a binary...

I agree. May not be the best solution to introduce gyp dependent packages (may be this a good candidate for unjs?).

After making some tests on internal repo (with nuxi change reverted), using this branch instead of chokidar drop watcher to be ready from ~30s to 300ms.

This also impact global nitro/vite startup time

Using Nuxt 3.4.0-28018829.ae5df72c:

Vite client warmed up in 16953ms      
Nitro built in 3414 ms                
[nuxt] builder:chokidar:watch: 1:05.603 (m:ss.mmm) 

Using this branch:

[nuxt] ready:layer1 322.7ms     
[nuxt] ready:layer2 322.676ms   
[nuxt] ready:layer3 322.576ms
Vite client warmed up in 9955ms    
Nitro built in 1252 ms          

@pi0
Copy link
Member

pi0 commented Apr 10, 2023

Nice for benchmarks! Are the above results on windows? I'm curious about linux/macos impact too.

We can make a similar watcher util with watchman but packaged similar to chokiar+fsevents and with an optional bin dependency (actually we might need two packages one the lib and one optional dep with node bindings). Feel free to try and making it is is more than welcome in unjs if finally proven to be better than chokidar.

@stafyniaksacha
Copy link
Contributor

stafyniaksacha commented Apr 10, 2023

Are the above results on windows

Yes, win 11 with wsl2 (out of DrvFS)

@pi0
Copy link
Member

pi0 commented Apr 10, 2023

wsl2 is linux kernel... (actually depends on the test path. testing watch perf on a linux storage vs windows mounts differs)

@danielroe
Copy link
Member Author

I would suggest raising an issue with @parcel/watcher with any concerns about packaging rather than duplicating the work.

@pi0
Copy link
Member

pi0 commented Apr 10, 2023

The js wrapper is super small. The main concerns I mentioned earlier require significant changes in the package distribution for binaries, which is probably hard to progress on a package already in use and much faster to try a small rebuild :)

@stafyniaksacha
Copy link
Contributor

stafyniaksacha commented Apr 10, 2023

I've found this commit in vite from 2 weeks ago that seem to be related: vitejs/vite@24b91d5

chokidar is used for build --watch

@danielroe danielroe marked this pull request as ready for review April 13, 2023 21:48
@danielroe danielroe requested a review from pi0 April 13, 2023 21:48
@danielroe danielroe changed the title perf: replace chokidar with @parcel/watcher perf: allow using @parcel/watcher for dev watcher Apr 14, 2023
@logotip4ik
Copy link
Contributor

Hello, tested on windows 10 machine (not wsl) with small to medium project

Chokidar (1 ... 5 runs):

  1. builder:chokidar:watch - 3.952s
    Vite client warmed up in 3931ms
    Nitro built in 1537 ms
  2. builder:chokidar:watch - 3.992s
    Vite client warmed up in 3954ms
    Nitro built in 1671 ms
  3. builder:chokidar:watch - 3.867s
    Vite client warmed up in 3780ms
    Nitro built in 1544 ms
  4. builder:chokidar:watch - 4.146s
    Vite client warmed up in 4076ms
    Nitro built in 1553 ms
  5. builder:chokidar:watch - 3.554s
    Vite client warmed up in 3892ms
    Nitro built in 1560 ms

@parcel/watcher:

  1. builder:parcel:watch - 10.545ms
    Vite client warmed up in 3081ms
    Nitro built in 1648 ms
  2. builder:parcel:watch - 22.323ms
    Vite client warmed up in 3087ms
    Nitro built in 1522 ms
  3. builder:parcel:watch - 25.336ms
    Vite client warmed up in 3139ms
    Nitro built in 1733 ms
  4. builder:parcel:watch - 27.146ms
    Vite client warmed up in 3055ms
    Nitro built in 1633 ms
  5. builder:parcel:watch - 28.613ms
    Vite client warmed up in 3078ms
    Nitro built in 1629 ms

  • Node Version: v18.13.0
  • Nuxt Version: 3.4.1
  • Nitro Version: 2.3.3
  • Package Manager: yarn@3.4.1
  • Builder: vite

Copy link
Member

@pi0 pi0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Only would be nice to make flag as experimental.newWatcher so that we can change experiment to different providers without introducing additional flags.

@danielroe
Copy link
Member Author

Good idea!

@danielroe danielroe merged commit a086af9 into main Apr 19, 2023
17 checks passed
@danielroe danielroe deleted the feat/parcel-watcher branch April 19, 2023 21:02
This was referenced Apr 19, 2023
This was referenced Apr 20, 2023
@Barbapapazes
Copy link
Contributor

Whoooo, that's absolutely game changer!

Here the project, it's a Nuxt Layer https://github.com/Classement-des-Associations/website-theme and I'm developing on Windows.

With Parcel, that's fast (like a normal nuxt project)
image

With Chokidar, that was unusable.
image
(After seven minutes, still not ready)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants