Makes writeValueRecursively iterative
#368
Merged
Conversation
This file contains 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
The method `_Private_IonWriterBase#writeValueRecursively` is recursive in two senses: 1. It visits all of the children in the provided IonReader's current value recursively. 2. The method itself is implemented using recursion. This patch modifies the implementation (#2) to be iterative, eliminating a source of StackOverflowExceptions and offering a modest reduction in CPU cost.
Codecov Report
@@ Coverage Diff @@
## master #368 +/- ##
=========================================
Coverage 64.04% 64.05%
+ Complexity 4838 4835 -3
=========================================
Files 136 136
Lines 21105 21108 +3
Branches 3818 3821 +3
=========================================
+ Hits 13517 13520 +3
+ Misses 6254 6251 -3
- Partials 1334 1337 +3
Continue to review full report at Codecov.
|
jobarr-amzn
reviewed
Jun 16, 2021
Comment on lines
+432
to
+433
| // The isNullValue() check above will handle this. | ||
| throw new IllegalStateException("isNullValue() was false but IonType was NULL."); |
There was a problem hiding this comment.
I wondered about this, thanks for the clarification
| stepIn(IonType.SEXP); | ||
| break; | ||
| default: | ||
| throw new IllegalStateException("Unknown value type: " + type); |
There was a problem hiding this comment.
Your mileage may vary here, but what do you think of this expression?
switch (type) {
case NULL: // The isNullValue() check above will handle this.
throw new IllegalStateException("isNullValue() was false but IonType was NULL.");
case BOOL: writeBool(reader.booleanValue()); break;
case INT: writeInt(reader.bigIntegerValue()); break;
case FLOAT: writeFloat(reader.doubleValue()); break;
case DECIMAL: writeDecimal(reader.decimalValue()); break;
case TIMESTAMP: writeTimestamp(reader.timestampValue()); break;
case STRING: writeString(reader.stringValue()); break;
case SYMBOL: writeSymbolToken(reader.symbolValue()); break;
case BLOB: writeBlob(reader.newBytes()); break;
case CLOB: writeClob(reader.newBytes()); break;
case STRUCT: // Intentional fallthrough
case LIST: // Intentional fallthrough
case SEXP: reader.stepIn(); stepIn(type); break;
default:
throw new IllegalStateException("Unexpected value type: " + type);
}There was a problem hiding this comment.
I adopted the fallthrough for the container types, thanks!
| */ | ||
| final void writeValueRecursively(IonType type, IonReader reader) | ||
| throws IOException | ||
| final void writeValueRecursively(IonType ionType, IonReader reader) |
There was a problem hiding this comment.
Zack responded offline that while the implementation is no longer recursive the value structure still is. This makes sense, traversing a recursive structure is what this does no matter how it does it.
|
Thank you for the detailed comments! |
tgregg
previously approved these changes
Jun 16, 2021
jobarr-amzn
approved these changes
Jun 16, 2021
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.
The method
_Private_IonWriterBase#writeValueRecursivelyisrecursive in two senses:
current value recursively.
This patch modifies the implementation to be iterative,
eliminating a source of StackOverflowExceptions and offering
a modest reduction in CPU cost.
Benchmark
This benchmark creates an
IonStructthat is 1,000 levels deep, wraps it in anIonReader, then useswriteValueRecursivelyto write it to a binary Ion stream 100k times. The source can be viewed here.This change does not affect allocations or garbage collection, only CPU time.
Before
After
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.