Repository navigation
Conversation
skip_over_whitespace() called skip_whitespace(), which unreads the first character after the whitespace, then read_char() to read it again. The text reader does this before every token. Return that character directly instead. The cases match the old code: a '/' that does not start a comment is returned as '/', EOF is returned as EOF, and read_char() still counts newlines.
UnifiedInputStreamX.unread() looked up the current page and its starting offset on every call to check whether the character being stepped back over is still in the current page. Cache that starting offset (_unread_floor) in make_page_current(), the one place a page becomes current. While a page is current its starting offset can only decrease (unreading before its first character lowers it), and a page is only reloaded by refill(), which makes it current again through make_page_current(), so the cached value stays an upper bound of the starting offset: a position above it is inside the page. When _pos is above it, unread() only decrements _pos; every other case takes the existing path.
Strings, symbol identifiers and skipped values were scanned one character at a time through UnifiedInputStreamX.read(). Add helpers that consume a run of characters directly from the current page: appendAsciiRun() copies accepted ASCII characters into a StringBuilder, skipRun() and skipAsciiRun() skip them. None of them refills; the existing character-at-a-time loop continues with the character that stopped the run, or with read() at the end of the page. Each table accepts exactly the characters the existing loop passes over without acting on them, so everything else (escapes, quotes, line ends and line counting, comments, brackets, non-ASCII) still goes through the existing code. Spaces and tabs are skipped with the rest when skipping a container, since the container scan ignores them. Used when loading double-quoted strings and symbol identifiers, and when skipping symbol identifiers, double-quoted strings, containers and blobs.
Blobs were decoded a byte at a time through read_base64_byte(), which reads each base64 character through UnifiedInputStreamX.read(). Decode groups of four alphabet characters directly from the current page into the lob buffer instead, for byte and character sources. Decoding stops at whitespace, '=' (which the alphabet table maps to -1), the closing brace, a non-ASCII or invalid character, or the end of the page, and leaves all of those to read_base64_byte(), so padding, wrapped base64 and errors behave as before. readBytes() goes back to bulk decoding after each such character whenever no decoded bytes are pending, never asks for more than the requested length, and still marks the token finished only when it reached the end of the lob.
For a field name, parse_to_next_value() finished the name token to a save point, which skips over it, and then read the name back by reactivating that save point. Each field name was scanned twice and paid for a save-point push and pop. When the name is an unquoted identifier and nothing is loaded yet, read it directly with token_contents_load(), which records the save point and marks the token finished, after making the same state change finish_value() made; every other case keeps the existing path. After the name the stream is now before the whitespace that follows it, not after it. The next nextToken() skips that whitespace either way, so spans and line numbers are unchanged. One diagnostic moves: a keyword used as a field name (true, false, null, nan) is now reported at the end of the name, where the stream is, not after the spaces and comments that follow it.
IonReaderTextFastPathsTest has two parts. Reader-level tests read strings, symbols and blobs longer than a page, skipped containers whose contents look like their closing bracket, skipped multi-line values, malformed blobs, prefix reads of cached blobs, field-name spans and errors, and random documents written by the text writer, from byte arrays, streams that return a few bytes per read (so pages end anywhere), strings and readers. They check results, not which path produced them, and pass before and after the fast paths; they would not notice a fast path no longer being called, which is left to benchmarks. Unit tests call the new UnifiedInputStreamX helpers and the bulk base64 read directly, and check that each consumes a whole run or whole groups and stops at the right character. Together the tests fail when any character table, the unread floor, a non-ASCII check, the base64 group decoding or the bulk base64 read is broken.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #1172 +/- ##
============================================
+ Coverage 67.23% 68.28% +1.04%
- Complexity 5484 5737 +253
============================================
Files 159 160 +1
Lines 23025 23519 +494
Branches 4126 4241 +115
============================================
+ Hits 15481 16059 +578
+ Misses 6262 6152 -110
- Partials 1282 1308 +26 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
This branch has not been deployed
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.
Issue #, if available: none
Description of changes:
Text Ion reads in
ion-java-benchmark-cliget 10–31% faster, with no API change and one minor diagnostic change (see below). The PR adds five independent fast paths to the text reader (IonReaderTextRawTokensX,IonReaderTextRawX,IonReaderTextSystemX,UnifiedInputStreamX), one commit each, plus a test commit. It's 264 added and 6 removed lines insrc/main(201 lines of code without comments and blank lines). Each fast path handles only the common case and falls back to the existing character-at-a-time code for everything else, so escapes, newlines and line counting, comments, non-ASCII, page ends, padding and errors keep their current behavior.skip_over_whitespace()calledskip_whitespace(), which unreads the first non-whitespace character, thenread_char()to read it again, before every token. It now returns that character directly.UnifiedInputStreamX.unread()looked up the current page and its starting offset on every call. That offset is now cached inmake_page_current(), the one place a page becomes current. While a page is current its starting offset can only decrease, and a page is only reloaded throughrefill(), which goes throughmake_page_current()again, so the cached value stays an upper bound of the starting offset: when_posis above it, stepping back stays inside the page. Other cases take the existing path.UnifiedInputStreamXhelpers (appendAsciiRun,skipRun,skipAsciiRun) consume a run of characters straight from the current page, without refilling. Each run uses a table holding exactly the characters the existing loop passes over without acting on them; the existing loop then continues with the character that stopped the run. Used for double-quoted strings and symbol identifiers (load and skip), and for skipping strings, containers and blobs. Line ends always stop a run, so line counting is unchanged.=, the closing brace, invalid characters and page ends are left toread_base64_byte(), after whichreadBytes()goes back to bulk decoding whenever no decoded bytes are pending.token_contents_load(), after the same state changefinish_value()made; other names keep the existing path.IonReaderTextFastPathsTest, described under Testing.Performance
These numbers come from
ion-java-benchmark-cliwith the settings ofion-java-performance-regression-detector.ymland its six generated datasets (50 KB each), plus--format ion_text. The workflow as written benchmarksion_binary, the CLI's default, so on this PR it won't exercise the text reader.Settings:
--format ion_text --mode AverageTime --forks 5 --warmups 2 --iterations 3 --io-type buffer, JDK 17. Benchmarks ran one at a time, before and after, on an otherwise idle 16-core x86_64 Linux r5.4xl host. Values are ms/op, lower is better, ± the 99.9% confidence interval; every change is outside both intervals.These datasets have few large blobs. For a single 8 MiB blob read with
newBytes()(rough timing, best of 5 after 2 warm-ups, not JMH), the time changes as follows:byte[]InputStreamInputStream, base64 wrapped at 76 charactersStringReaderTo reproduce, build the CLI against each side and run, for each dataset:
Testing
./gradlew buildpasses: SpotBugs, Spotless, ktlint and the third-party license check.testandminifyTest8,minifyTest11,minifyTest17andminifyTest21each give 197,949 passing and 586 skipped, the same skipped set as without the change.IonReaderTextFastPathsTest(61 cases) has two parts.Its reader-level tests read their inputs from byte arrays, streams that return a few bytes per read (so pages end anywhere), strings and readers. They cover:
\n,\r\nand\rbyteSize(), onegetBytes()of the first 1 to 8 bytes, which come from the cached blob, then the next valueThey check results, not which path produced them, and pass before and after the change.
Its unit tests call the new
UnifiedInputStreamXhelpers and the bulk base64 read directly, and check that each consumes a whole run or whole groups and stops at the right character. This includes a bulk read shorter than the blob, which leaves the following decoded byte toread_base64_byte().Together they fail when any character table (line ends included), the unread floor, a non-ASCII check, the base64 group decoding, the bulk base64 read or the character-source copy is broken.
Outside this PR, the old and new reader were compared on 20,000 random documents and 8 MB of real data, each read four ways. That covered comments, newline forms, escapes, surrogate pairs, page-sized values, wrapped base64, clobs, truncation and corrupted bytes. Values, field names, annotations, spans, and exceptions with their messages were identical.
Notes for reviewers
true,false,null,nan) is now reported at the end of the name instead of after the spaces and comments that follow it. For example,{true :1}reports offset 6 instead of 7, and{x:{nan /*c*/ : 1}}offset 8 instead of 15. The exception type and message are unchanged, and when the name is directly followed by:or a line end the offset is unchanged too. Keeping the old offset would take extra code on the error path for no benefit to callers, so this PR doesn't; the test pins the new offsets.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.