Repository navigation
Conversation
Options that change how tokens compare (ignoreCase; ignoreWhitespace and ignoreNewlineAtEof in diffLines; the trailing-comma rule in diffJson; the always-on trim in diffWords) were applied inside equals(), to both tokens, on every call. The diff loop calls equals() many times per token (~3M calls diffing two versions of npm's package-lock.json). Add a normalizeToken() hook to Diff that runs once per token before diffing. The diff loop compares the normalized tokens, while change objects are still built from the original ones. The base implementation handles ignoreCase, and LineDiff, JsonDiff and WordDiff build on it via super.normalizeToken(), so none of them override equals() any more. Diff output is unchanged (fuzzed against the previous build across all diff functions and option combinations). Behaviour changes, all outside documented usage: - A comparator now receives normalized tokens, e.g. lowercased with ignoreCase. Previously ignoreCase was skipped when a comparator was given, which looks like an artifact of the implementation rather than intended behaviour. - diffWords now honours a comparator; WordDiff.equals() ignored it. - Calling equals() directly (e.g. lineDiff.equals()) no longer applies these options, since they're now applied by normalizeToken(). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The previous commit made normalizeToken() call toLowerCase() on every token when ignoreCase is set. diffArrays tokens can be any value, so diffArrays with ignoreCase on non-string items now threw before diffing started. ignoreCase isn't a diffArrays option, but it could be passed from JS anyway, and the old equals() only lowercased tokens that weren't already ===. So diffing identical arrays of non-strings with ignoreCase used to work, and a call that worked shouldn't start throwing. Leave non-string tokens alone instead. As a side effect, diffing non-identical arrays of non-strings with ignoreCase, which used to throw, now works too. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Add normalizeToken to the list of methods a Diff subclass can override, and update the equals entry: it now receives normalized tokens, and its default also uses the comparator option, which it already did but the README didn't mention. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
ce8d264 to
d82e115
Compare
|
Haven't reviewed the code yet but will comment to note that in concept this sounds good, and also that it happily helps enable a (perhaps more dramatic) further optimization (see #736). Only thing I need to think about is whether increased memory use is a strike against this, and how to characterise the memory impact in the release notes. (I guess that potentially this roughly doubles peak memory use, in the case where the tokens themselves are large strings and therefore dominate the memory use? My thinking is that previously we'd only have needed to hold on to normalized versions of two of those strings at a time before we exit |
toLowerCase() allocates a new string even when the token is already lowercase, and the normalized tokens now live for the whole diff. Reusing the original token means only tokens with uppercase letters cost extra memory. On lines with long, mostly-lowercase tokens this cut Node's peak memory with ignoreCase from ~135 MB to ~106 MB (master: ~83 MB).
|
Good question, I measured it. In short: you're right about the mechanism, but in practice it only matters for options that actually rewrite tokens (mainly Where the extra memory comes from
Peak RSS growth during one diff, in MB (median of 3 runs, Node 26.10 / Bun 1.4.4, with 835507c)
At realistic sizes the difference is within noise, apart from about +3 MB for Things I tried that didn't help
The only way to go further would be to stop keeping normalized strings during the diff at all, for example by hashing each normalized token and only re-normalizing to confirm a match when two hashes are equal. That's a bigger change, needs a fallback path for Possible release-notes wording
|
| // Only strings have a case. Tokens can be any value in diffArrays, which has no ignoreCase | ||
| // option but may still be passed one, so leave other tokens alone rather than throwing. | ||
| if (options.ignoreCase && typeof token === 'string') { | ||
| // toLowerCase() returns a new string even when nothing changed (at least in V8), and the |
There was a problem hiding this comment.
I don't think this is true? My Claude disagrees, and if I test with %DebugPrint I see the result of x.toLowerCase() having the same memory address as x:
mark@fractal:~$ node --allow-natives-syntax
Welcome to Node.js v22.14.0.
Type ".help" for more information.
> x = 'hello world'
'hello world'
> y = x.toLowerCase()
'hello world'
> %DebugPrint(x)
DebugPrint: 0x3a4243dea151: [String] in OldSpace: #hello world
0x3999b7cc0691: [Map] in ReadOnlySpace
- map: 0x3999b7cc0841 <MetaMap (0x3999b7cc0099 <null>)>
- type: INTERNALIZED_ONE_BYTE_STRING_TYPE
- instance size: variable
- elements kind: HOLEY_ELEMENTS
- enum length: invalid
- stable_map
- non-extensible
- back pointer: 0x3999b7cc0069 <undefined>
- prototype_validity cell: 0
- instance descriptors (own) #0: 0x3999b7cc0c91 <DescriptorArray[0]>
- prototype: 0x3999b7cc0099 <null>
- constructor: 0x3999b7cc0099 <null>
- dependent code: 0x3999b7cc0c51 <Other heap object (WEAK_ARRAY_LIST_TYPE)>
- construction counter: 0
'hello world'
> %DebugPrint(y)
DebugPrint: 0x3a4243dea151: [String] in OldSpace: #hello world
0x3999b7cc0691: [Map] in ReadOnlySpace
- map: 0x3999b7cc0841 <MetaMap (0x3999b7cc0099 <null>)>
- type: INTERNALIZED_ONE_BYTE_STRING_TYPE
- instance size: variable
- elements kind: HOLEY_ELEMENTS
- enum length: invalid
- stable_map
- non-extensible
- back pointer: 0x3999b7cc0069 <undefined>
- prototype_validity cell: 0
- instance descriptors (own) #0: 0x3999b7cc0c91 <DescriptorArray[0]>
- prototype: 0x3999b7cc0099 <null>
- constructor: 0x3999b7cc0099 <null>
- dependent code: 0x3999b7cc0c51 <Other heap object (WEAK_ARRAY_LIST_TYPE)>
- construction counter: 0
'hello world'
Am I missing something? If not, this optimisation definitely seems misguided!
There was a problem hiding this comment.
TL;DR: On current V8 (Node 24+), toLowerCase() copies strings even when nothing changes once they're longer than 24 characters (and non-ASCII strings get copied on every Node version), so the fix saves real memory there, while on other runtimes it's a cheap no-op.
Okay, so that was a rabbit hole! I had benchmarked the fix as actually using less memory, so your test confused me, and when I reran it, Node 26 showed no copy either. So why did the fix reduce memory usage?
I ended up looking at the V8 source code (with Claude's help). toLowerCase() has a fast path, used for strings of at most 24 characters (such as "hello world") with no characters above U+00FF, and it still returns the original if nothing changed. Anything longer goes down another code path, which always copies. That includes slices and joined strings from 13 characters up, which is exactly what diffLines tokens are. Strings with characters above U+00FF (€, 中, emoji) go to ICU, which always copies, at any length.
So I reran your test with a 25+ character string and got the copying behaviour that warrants the fix. But before responding I reran it on Node 22, and there it didn't copy!
That led me to V8 commit d8fd49d (March 2025). It improves toUpperCase() by checking whether there's anything to convert before creating the copy, and so removes the "return the original after copying" check from the shared helper. But it doesn't add the same improvement for toLowerCase(), which lost its check too, and the review even mentions this:
I removed change detection support from the FastConvert case as well now since we really shouldn't convert at all in that case. I didn't add it to ToLower yet though, since it's a bit messy due to direct calls from CSA...
This shipped in V8 13.6, i.e. Node 24. The old check only ever covered pure ASCII, though, so non-ASCII strings past the fast path were already copied on Node 22. Here's x.toLowerCase() on an already-lowercase x, checked with %DebugPrint:
x |
Node 22 | Node 24+ |
|---|---|---|
| ≤ 24 chars, no characters above U+00FF | original | original |
> 24 chars (or a diffLines token ≥ 13 chars), ASCII |
original | copy |
same, but with Latin-1 (é) |
copy | copy |
| any character above U+00FF, any length | copy | copy |
Extra heap for the normalized tokens of diffLines(…, { ignoreCase: true }) on yarn.lock, without → with the fix:
| Input | Node 22 | Node 24+ |
|---|---|---|
yarn.lock (8.5k lines) |
194 KB → 196 KB | 432 KB → 192 KB (−56%) |
| every line repeated 50× (17 MB) | 5.1 MB → 5.1 MB | 17.7 MB → 5.1 MB (−71%) |
Current V8 main still copies. I couldn't find any reported issue on this, so I'm thinking of reporting it, to be honest, though I've never reported an issue to Chromium before.
So I'd argue the fix (comparing ourselves and returning the original where appropriate) is useful on every V8-based runtime until this is fixed, and for non-ASCII text even on older ones, and it shouldn't hurt anything else: on Bun it's a no-op, and I couldn't measure any difference in speed.
The comment said toLowerCase() always returns a new string in V8. It only does for longer or non-ASCII strings; short ASCII strings come back unchanged. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
I had another look at interning the values as we normalize them, and claude drafted (I haven't cleaned it up at all) a branch which interns all values using a hash table. Interning keeps only one copy of each distinct normalized string, so with One thing to note is that interning is not valid when a consumer is using .comparator or their own overriden One unexpected finding is that #734 actually already reduces peak memory, and in most scenarios, it reduces more than interning does. In my first version, which used string I think this PR can be merged as is (no interning, optimize memory where appropiate), and #734 could be reviewed and merged, and then interning/hashing could be investigated later, or perhaps as a precursor to the token discarding PR discussed in #736 |
diffLineswithignoreWhitespacecalledtrim()on both lines every timeequals()was called, and the diff loop callsequals()many times per token. Trimming each line once up front made large diffs withignoreWhitespaceabout twice as fast.The other diffs had the same pattern:
ignoreCase,ignoreNewlineAtEof,diffJson's trailing-comma regex anddiffWords' trim all ran insideequals(). So this PR moves all of them into a newnormalizeToken()hook that runs once per token.equals()now compares the normalized tokens. The returned change objects still contain the original text, and diff output is unchanged (fuzz-tested againstmasteracross all diff functions and option combinations).The README documents
normalizeToken()alongside the other methods aDiffsubclass can override.Benchmarks
Median ms,
mastervs this PR:diffLines(no options)diffLinesignoreWhitespacediffLinesignoreNewlineAtEofdiffLinesignoreCasediffJsondiffWordsdiffWordsignoreCasediffChars(no options)diffCharsignoreCasediffSentences(no options)diffArrays(no options)Cases where nothing needs normalizing (
diffLines,diffSentencesanddiffArrayswithout options) are not slower.Inputs, all from this repo's history:
yarn.lockbefore and after ff14775 (8.5k lines, ~800 changed)diffJson: the same two files parsed into objectsREADME.mdat 9e2da01 and 8d706c8release-notes.mdat the same two commitsNode 26.10.0, 5 alternating runs per build.
Behaviour changes
A
comparatornow receives normalized tokensWith
ignoreCase, it gets lowercased tokens. PreviouslyignoreCasewas silently skipped whenever acomparatorwas passed.comparatoris only documented and typed fordiffArrays, which has noignoreCaseoption, and I found nothing in the docs, tests, issues or PRs about combining the two. I'm assuming the old behaviour was a side effect of howequals()was written, not a design decision. (diffLinesanddiffJsonalready passed their trimmed or comma-stripped tokens to a comparator; only the lowercasing is new.)diffWordsnow honours acomparatorPreviously its
equals()never called it.comparatorisn't a documenteddiffWordsoption either.equals()receives normalized tokensA subclass that overrides
equals()still works, but now gets, for example, lowercased tokens whenignoreCaseis set. Callingequals()directly on the exported instances (lineDiff.equals(...)etc.) no longer applies options likeignoreWhitespace, becausenormalizeToken()applies them now. I'm assuming direct calls are rare, since they aren't documented.diffArrays
withignoreCase` on non-string items no longer throwswhen the arrays differ.
ignoreCasenow leaves non-string tokens alone.