fix: hand out a recorded hash update as buffers - #289
Conversation
CachedSource.updateHash records what the wrapped source hashed so later calls replay it. Since 3.5.2 a string-backed source hashes its string, so the recording keeps that string, and getCachedData() handed it to callers that serialize it — webpack's pack warns "Serializing big strings" and pays for it on every deserialization. Encode the strings as buffers in getCachedData(), the boundary a caller serializes from, so the recording still holds the source's own string and nothing copies it until it is written out. Hashing reads a string as utf8, so the restored update hashes to the same digest.
🦋 Changeset detectedLatest commit: 9e47885 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 2 included reviews per hour; 0 remain after this review. WalkthroughWhen Merge Risk: ⚪ Minimal · up to The change stores cached hash updates as UTF-8 buffers, which the supported hash implementations treat equivalently to strings. No material merge risk remains. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Some tools did not complete. Review the errors below. 🔧 ESLint
lib/CachedSource.jsESLint failed to execute (timeout). test/CachedSource.jsESLint skipped: the matched ESLint configuration already failed (timeout). Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #289 +/- ##
=======================================
Coverage 98.29% 98.30%
=======================================
Files 25 25
Lines 2289 2295 +6
Branches 735 738 +3
=======================================
+ Hits 2250 2256 +6
Misses 37 37
Partials 2 2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Merging this PR will degrade performance by 31.45%
|
The first version rebuilt the buffered array on every getCachedData() call, so a source serialized more than once was re-encoded each time: +19.7MB of buffer allocation over the css/large fixture's cache build. Encode in place instead. A source nobody asks cached data of still keeps its own string uncopied, and one that is handed out repeatedly is encoded once.
1 participant
Summary
Fixes a regression 3.5.2 (#286) introduced in webpack's persistent cache.
CachedSource.updateHashrecords what the wrapped source hashed so later calls replay it. Before 3.5.2 a string-backed source hashed its utf8 buffer, so the recording held aBuffer; since 3.5.2 it hashes the string directly, so the recording holds that string — andgetCachedData()handed it straight to callers that serialize it. webpack's pack serializer then warnsSerializing big strings (1922kiB) impacts deserialization performanceand pays that cost on every deserialization; it turns webpack'sconfigCases/css/largered underConfigCacheTestCases(e.g. webpack/webpack#22299).The fix encodes those strings as buffers in
getCachedData()— the boundary a caller serializes from — so the recording still holds the source's own string and nothing is copied until it is written out. 3.5.2's memory win is kept: no buffer is materialized while hashing, only when cached data is handed out. Hashing reads a string as utf8, so a restored update hashes to the same digest.What kind of change does this PR introduce?
fix
Did you add tests for your changes?
Yes —
test/CachedSource.jsassertsgetCachedData().hashholds no string, that the recording still keeps the source's own string uncopied, and that aCachedSourcerestored from that data hashes to the same digest. It fails without the change.Verified against webpack: with this build,
configCases/css/largepasses and the wholeConfigCacheTestCasessuite emits no big-string warning (only the twoprofiling-plugincases fail, for unrelated sandbox reasons).Does this PR introduce a breaking change?
No. The on-disk shape returned by
getCachedData()goes back to what 3.5.1 produced.If relevant, what needs to be documented once your changes are merged or what have you already documented?
n/a
Use of AI
AI was used to root-cause the regression from webpack's failing cache suite, write the test and the fix, and verify both suites; the result was reviewed in-session.
Generated by Claude Code
Summary by CodeRabbit