Fix/scanner quadratic scanning - #1
Merged
Merged
Conversation
The scanner addresses text by character offset. Resolving a character
offset against a UTF-8 string means walking it from the start, so calling
mb_substr() once per character made scanning cost time proportional to the
square of the document length. Both the per-character reads in
StringHelper::charCodeAt() and the per-token reads in
StringHelper::substring() paid it.
Split the document into characters once in the constructor and index that
array instead. charCodeAt() takes an ord() fast path for single-byte
characters, which covers essentially all of a JSON document's structure.
Memory cost is roughly 46x the source size, paid once per scanner.
The text property is no longer read now that every access goes through the
character array, so it is gone.
Measured over a document shaped like a Shopify theme locale file, running
parse + modify + applyEdits:
8 KB 55 ms -> 9.3 ms
33 KB 720 ms -> 22.4 ms
90 KB 5,307 ms -> 61.4 ms
183 KB 21,384 ms -> 128.0 ms
Scaling is linear after the change: twice the input costs 2.1x the time.
The new complexity tests assert a ratio between two input sizes rather than
a wall-clock budget, so they carry the same meaning on any machine.
Quadratic scanning takes ~16x as long for 4x the input and linear scanning
takes ~4x, which leaves a wide margin around the threshold of 8. The second
test guards against a fix that speeds up ASCII by falling back to byte
offsets while leaving multibyte documents quadratic.
The scanner reports offsets and lengths in characters rather than bytes, and the editor depends on that agreement: when the two disagreed, an edit after a multibyte character landed at the wrong position and mangled the output. The editor tests cover that end to end, but the scanner itself had no multibyte tests at all, so a change to how it addresses characters failed a long way from its cause. These assert offsets, token values, lengths, line and column numbers, and the token stream itself across 2-, 3- and 4-byte characters, Greek, Thai, Cyrillic, CJK and emoji. They pass against the scanner both before and after the switch to indexing a character array, which is the point: they describe behavior that was already correct rather than behavior the change introduced.
Merged
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
1 participant
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.
No description provided.