Skip to content

perf: limit the number of concurrently open file watchers on macOS#249

Merged
alexander-akait merged 3 commits intowebpack:mainfrom
chenjiahan:watcher_macos_perf_0429
May 19, 2025
Merged

perf: limit the number of concurrently open file watchers on macOS#249
alexander-akait merged 3 commits intowebpack:mainfrom
chenjiahan:watcher_macos_perf_0429

Conversation

@chenjiahan
Copy link
Contributor

@chenjiahan chenjiahan commented Apr 29, 2025

Limit the number of concurrently open file watchers on macOS to 20 to make FSWatcher.close faster.

Related PRs:

Related issues:

This was referenced Apr 29, 2025
Open
Merged
alexander-akait
alexander-akait reviewed Apr 29, 2025
View reviewed changes
Copy link
Member

@alexander-akait alexander-akait left a comment

Choose a reason for hiding this comment

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

I am fine with it, but can we add a test case/simple perf case to verify it and to avoid future regressions?

@codecov
Copy link

codecov bot commented Apr 29, 2025 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.46%. Comparing base (33df387) to head (e3426af).
Report is 1 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main     #249      +/-   ##
==========================================
- Coverage   92.20%   91.46%   -0.75%     
==========================================
  Files           6        6              
  Lines        1065     1066       +1     
  Branches      258      258              
==========================================
- Hits          982      975       -7     
- Misses         83       91       +8
Flag Coverage Δ
integration 91.46% <100.00%> (-0.75%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@chenjiahan
Copy link
Contributor Author

chenjiahan commented Apr 29, 2025

Sure, creating a stable perf test seems difficult, so I added a simple test to check the watcher limit value

@alexander-akait
Copy link
Member

alexander-akait commented Apr 29, 2025

@chenjiahan very weird, looks like Node.js@22 has regression somewhere on windows... I know it is not related to these changes... I will look deeply

@alexander-akait
Copy link
Member

alexander-akait commented May 16, 2025

@chenjiahan sorry for delay, can you rebase?

@alexander-akait
Copy link
Member

alexander-akait commented May 16, 2025

We fixed problem with node@22

@chenjiahan chenjiahan force-pushed the watcher_macos_perf_0429 branch from 5b3deb8 to e3426af Compare May 17, 2025 04:15
@chenjiahan
Copy link
Contributor Author

chenjiahan commented May 17, 2025

Rebased 😄

@alexander-akait alexander-akait merged commit e7eaa50 into webpack:main May 19, 2025
45 of 46 checks passed
@chenjiahan chenjiahan deleted the watcher_macos_perf_0429 branch May 19, 2025 23:58
@chenjiahan chenjiahan mentioned this pull request May 20, 2025
Merged
2 tasks
@chenjiahan chenjiahan mentioned this pull request Jun 19, 2025
Closed
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@alexander-akait alexander-akait alexander-akait approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@chenjiahan @alexander-akait