fix(worddiff): guard intra-line diff cost by LCS table size, not line length - #328
Merged
Conversation
… length The 500-byte per-line cap skipped word-diff on any pair where either side was longer, so a markdown paragraph living on one physical line silently got no highlighting while the status bar still showed the mode as on. Byte length is a poor proxy for the cost it was guarding: 500 repeated letters tokenize to a single token, 500 bytes of minified JSON to nearly 300. Gate on the LCS table size instead — minus tokens times plus tokens, budget 4M cells, about 32MB and 17ms for one pair, paid once per file load or word-diff toggle. The byte cap stays only as a cheap pre-filter at 20000 bytes so minified input never reaches the tokenizer. Related to #323
…s godoc The byte pre-filter subtest used two identical prose lines, which the cell budget and the identical-lines branch both reject on their own — it passed with the pre-filter deleted. It now uses a single-token pair that clears every other gate, with a control pair sized to fit the pre-filter, so the assertion can only be answered by the byte gate. The maxDiffCells godoc said the budget was paid once per file load. It bounds one pair; recomputeIntraRanges calls ComputeIntraRanges once per paired remove/add line, so a diff holding many long pairs pays it for each. Related to #323
TestChangedRanges_SkipsVeryLongLines still carried the pre-change rationale, a near-copy of the godoc this branch replaced, so the package held two contradictory reasons for one constant. The byte cap is a tokenizer pre-filter; maxDiffCells guards the LCS cost. The fixture below the comment is its own counter-example: 20001 repeated letters is a single token. Also correct prose's godoc, which returns exactly nbytes rather than roughly. Related to #323
Deploying revdiff with
|
| Latest commit: |
8264806
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://f630873e.revdiff.pages.dev |
| Branch Preview URL: | https://worddiff-cost-budget.revdiff.pages.dev |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
word-diff skipped any add/remove pair where either line was over 500 bytes, bailing out before tokenization. On markdown where a paragraph is one physical line that is most of the interesting pairs, so the status bar showed
±while nothing highlighted, which looks exactly like the feature being broken.Byte length was never the cost driver. The LCS table is, and it is quadratic in tokens, so the two are only loosely related: 500 repeated letters tokenize to a single token, 500 bytes of minified JSON to nearly 300. The old cap admitted at most 15,625 cells.
Measured here, prose input:
So the guard now budgets the table instead of the line:
maxDiffCells = 4_000_000(~32 MB, ~17 ms per pair), with the byte cap kept only as a cheap pre-filter at 20000 bytes so minified input never reaches the tokenizer.ComputeIntraRangeskeeps its signature, nothing is configurable, and no doc or plugin surface mentions the cap.recomputeIntraRangesruns once per file load and once perWpress, not per keystroke, so this is not on the render path.Tests pin each guard on its own: deleting the byte pre-filter fails one subtest, deleting the cell budget fails another, and
TestComputeIntraRanges_LongProsePairIsDiffedfails against the old 500-byte cap. The previous fixture for the byte gate used two identical prose lines, which three separate branches rejected independently, so it would have passed with the gate deleted.Not done here, both worth a separate decision:
--word-diff-max-lineas asked in the issue. Precedent exists (--compact-context,--page-overlap), but the knob is in bytes while the cost is quadratic in tokens, so there is no value a user could pick sensibly.ComputeIntraRangesto report why it returned nil, which changes thewordDifferinterface.One thing this widens without creating:
recomputeIntraRangeshas no budget across pairs, so a file with many long changed lines pays the per-pair cost repeatedly on the update goroutine. Reaching a visible stall needs roughly half a megabyte of changed content in one file with word-diff on.Related to #323