Skip to content

Grow reader buffer incrementally and add GZIP opt-out#1153

Merged
NovemberZulu merged 2 commits into
amazon-ion:masterfrom
deirdresama:fix-embargo-2
Jun 24, 2026
Merged

Grow reader buffer incrementally and add GZIP opt-out#1153
NovemberZulu merged 2 commits into
amazon-ion:masterfrom
deirdresama:fix-embargo-2

Conversation

@deirdresama

Copy link
Copy Markdown
Collaborator
  • IonCursorBinary grows the buffer by doubling as data is consumed, rather than allocating the full declared length up front
  • Add IonReaderBuilder.withGzipDecompressionEnabled to allow disabling automatic GZIP decompression

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@codecov

codecov Bot commented Jun 18, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 86.20690% with 4 lines in your changes missing coverage. Please review.
✅ Project coverage is 67.99%. Comparing base (3c1b6b1) to head (1766f2c).
⚠️ Report is 149 commits behind head on master.

Files with missing lines Patch % Lines
...main/java/com/amazon/ion/impl/IonCursorBinary.java 66.66% 2 Missing and 2 partials ⚠️
Additional details and impacted files
@@             Coverage Diff              @@
##             master    #1153      +/-   ##
============================================
+ Coverage     67.23%   67.99%   +0.76%     
- Complexity     5484     5632     +148     
============================================
  Files           159      160       +1     
  Lines         23025    23316     +291     
  Branches       4126     4182      +56     
============================================
+ Hits          15481    15854     +373     
+ Misses         6262     6168      -94     
- Partials       1282     1294      +12     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@tgregg

tgregg commented Jun 24, 2026

Copy link
Copy Markdown
Contributor

Revision 2

See only what changed: 1766f2c

Summary of changes:

IonCursorBinary.java

  1. Restored power-of-two buffer sizing in ensureCapacity()
  • Changed the growth expression from plain doubling Math.min(Math.max(capacity * 2L, capacity + 1L), max) to Math.min(nextPowerOfTwo((int)(capacity + 1)), max).
  • Reason: Revision 1's fix correctly decoupled allocation from the declared length, but as a side effect it dropped the long-standing property that the buffer stays a power of two (likely as either an actual or desperately-hoped-for performance optimization, but without remembering which I don't want to change it in the absence of some performance testing). Rounding up from the current capacity (not the requested length) keeps allocation safe while restoring power-of-two sizing. Note: refill()'s growth is fine to remain as plain doubling because ensureCapacity() is always called first, so refill() always receives a buffer with size that is already a power of two.
  1. Updated comments in both ensureCapacity() and refill()
  • Corrected refill()'s "after data has been read" wording (growth actually happens at the top of the loop, before that iteration's read), and documented the division of labor: ensureCapacity rounds to a power of two, refill doubles.
  • Reason: The original comment was imprecise, and the deliberate asymmetry between the two growth sites needed to be explained so it doesn't read as an accident.

IonReaderBuilder.java

  1. DRY'd up getInputStreamInterceptors()
  • Collapsed two identical GZIP-filtering loops (one for the detected list, one for the custom list) into a single loop after selecting the source list.
  • Reason: Revision 1's added branch duplicated the filtering logic; the only difference was which list it iterated.
  1. Updated Javadoc for the GZIP opt-out
  • addInputStreamInterceptor ("always consulted first" → "normally"), getInputStreamInterceptors, and the three build(...) methods ("will auto-detect and uncompress GZIPped Ion data" → "...unless disabled via
    withGzipDecompressionEnabled").
  • Reason: Those docs unconditionally promised GZIP auto-detection, which is no longer always true once the toggle can disable it. Note: IonSystem/IonLoader don't expose the toggle, so their docs were left
    unchanged.

IonCursorBinaryTest.java

  1. Strengthened the positive growth test with an exact buffer-size assertion
  • Added assertEquals(128, cursor.buffer.length).
  • Reason: Converts an indirect proof into a direct one that verifies power-of-two doubling (8→16→32→64→128).
  1. Corrected the length-bomb tests to exercise the fix
  • Changed the declared length from ~2 GB (which was above the max buffer size, due to ARRAY_MAXIMUM_SIZE being slightly below 2GB) to 1,000,000 (well below the max), and changed the expectation from assertThrows(IonException) to assertEquals(NEEDS_DATA, ...) plus assertThat(buffer.length, lessThanOrEqualTo(32)).
  • Reason: The revision 1 tests hit the pre-existing > maximumBufferSize path that deems the value "oversized", which the PR doesn't change, so they passed on master too. The sub-max case routes through
    the PR's new incremental-growth code. I verified by swapping in master's source: the rewritten tests fail there (buffer.length = 1,048,576 = nextPowerOfTwo(1,000,000)) and pass on the branch.
  1. Added annotation-wrapper variants of both bomb/growth tests
  • Reason: To exercise the same allocation paths when the declared length is reached through an annotation wrapper header, not just a top-level value. Refactored the tests to remain DRY via parameterization (@MethodSource).

@NovemberZulu NovemberZulu merged commit 4f33759 into amazon-ion:master Jun 24, 2026
31 of 38 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants