Skip to content

perf(caching): improves caching of loader and optimizer #464

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

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

OlenDavis
Copy link
Contributor

This PR contains a:

  • bugfix
  • new feature
  • code refactor
  • test update
  • typo fix
  • metadata update

Motivation / Use-Case

1. Performance refactor / race-condition fix:

This PR eliminates the race condition where the loader or optimizer may do the same processing (same transformation on the same image) redundantly if the same file transformation's loader is triggered more than once before the first transformation has finished.

2. Fix npm start

Didn't work before; now it does. (With as little change to the prior scripts as possible.)

Breaking Changes

None that I'm aware of.

Additional Info

This PR doesn't eliminate all possible such race conditions, namely if the same image transformation occurs between the loader and the minimizer, because the transformers are different functions (because the minimizer functionality is different than the generator/loader, even though the underlying operation is the same), it's still possible to have redundant image transformations if one transformation happens via the loader, and the same transformation happens via the webpack optimize hook.

Copy link

codecov bot commented Jan 18, 2025

Codecov Report

Attention: Patch coverage is 92.30769% with 2 lines in your changes missing coverage. Please review.

Project coverage is 78.15%. Comparing base (3a924e4) to head (b738fa4).
Report is 31 commits behind head on master.

Files with missing lines Patch % Lines
src/loader.js 87.50% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #464      +/-   ##
==========================================
+ Coverage   77.16%   78.15%   +0.98%     
==========================================
  Files           4        4              
  Lines         727      769      +42     
  Branches      282      304      +22     
==========================================
+ Hits          561      601      +40     
- Misses        137      139       +2     
  Partials       29       29              

☔ 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.

logger.debug(`loader cache miss: ${filename}`);

return worker(minifyOptions);
});
Copy link
Member

Choose a reason for hiding this comment

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

We can't do it in loader:

  1. this._compilation.getCache is unavaliable in multi threading mode (thread-loader)
  2. Also this plugin is using by rspack too and such api is not working there

This PR eliminates the race condition where the loader or optimizer may do the same processing (same transformation on the same image) redundantly if the same file transformation's loader is triggered more than once before the first transformation has finished.

Can you provide reproducible example?

Also I think we can use WeakMap in the loader to prevent such situations

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

Successfully merging this pull request may close these issues.

2 participants