From 24648f0ac8d7182752d4c600b213fedb4864c741 Mon Sep 17 00:00:00 2001 From: Zack Slayton Date: Wed, 16 Jun 2021 09:25:55 -0400 Subject: [PATCH 1/3] Makes `writeValueRecursively` iterative 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. --- .../ion/impl/_Private_IonWriterBase.java | 202 ++++++++++-------- 1 file changed, 114 insertions(+), 88 deletions(-) diff --git a/src/com/amazon/ion/impl/_Private_IonWriterBase.java b/src/com/amazon/ion/impl/_Private_IonWriterBase.java index 82802d938a..957d662296 100644 --- a/src/com/amazon/ion/impl/_Private_IonWriterBase.java +++ b/src/com/amazon/ion/impl/_Private_IonWriterBase.java @@ -44,10 +44,6 @@ public abstract class _Private_IonWriterBase static final String ERROR_FINISH_NOT_AT_TOP_LEVEL = "IonWriter.finish() can only be called at top-level."; - private static final boolean _debug_on = false; - - - /** * Returns the current depth of containers the writer is at. This is * 0 if the writer is at top-level. @@ -329,7 +325,6 @@ private final void write_value_field_name_helper(IonReader reader) } setFieldNameSymbol(tok); - if (_debug_on) System.out.print(":"); } } @@ -340,7 +335,6 @@ private final void write_value_annotations_helper(IonReader reader) // because local symtab diversion leaves the $ion_symbol_table // dangling on the system writer! TODO fix that, it's broken. setTypeAnnotationSymbols(a); - if (_debug_on) System.out.print(";"); } @@ -360,96 +354,128 @@ public void writeValue(IonReader reader) throws IOException } /** - * Unoptimized copy. This must not recurse back to the public - * {@link #writeValue(IonReader)} method since that will cause the - * optimization test to happen repeatedly. + * Writes the provided IonReader's current value including any annotations. This function will not advance the + * IonReader beyond the end of the current value; users wishing to continue using the IonReader at the current + * depth will need to call {@link IonReader#next()} again. + * + * - If the IonReader is not positioned over a value (for example: because it is at the beginning or end of a + * stream), then this function does nothing. + * - If the current value is a container, this function will visit all of its child values and write those too, + * advancing the IonReader to the end of the container in the process. + * - If both this writer and the IonReader are in a struct, the writer will write the current value's field name. + * - If the writer is not in a struct but the reader is, the writer will ignore the current value's field name. + * - If the writer is in a struct but the IonReader is not, this function throws an IllegalStateException. + * + * @param reader The IonReader that will provide a value to write. + * @throws IOException if the provided IonReader throws an IOException. + * @throws IllegalStateException if this writer is inside a struct but the IonReader is not. */ - final void writeValueRecursively(IonType type, IonReader reader) - throws IOException + final void writeValueRecursively(IonType ionType, IonReader reader) + throws IOException { - write_value_field_name_helper(reader); - write_value_annotations_helper(reader); + // The IonReader does not need to be at the top level (getDepth()==0) when the function is called. + // We take note of its initial depth so we can avoid advancing the IonReader beyond the starting value. + int startingDepth = getDepth(); + + // The IonReader will be at `startingDepth` when the function is first called and then again when we + // have finished traversing all of its children. This boolean tracks which of those two states we are + // in when `getDepth() == startingDepth`. + boolean alreadyProcessedTheStartingValue = false; + + // The IonType of the IonReader's current value. + IonType type; + + while (true) { + // Each time we reach the top of the loop we are in one of three states: + // 1. We have not yet begun processing the starting value. + // 2. We are currently traversing the starting value's children. + // 3. We have finished processing the starting value. + if (getDepth() == startingDepth) { + // The IonReader is at the starting depth. We're either beginning our traversal or finishing it. + if (alreadyProcessedTheStartingValue) { + // We're finishing our traversal. + break; + } + // We're beginning our traversal. Don't advance the cursor; instead, use the IonType that was + // passed in by the caller. + type = ionType; + // We've begun processing the starting value. + alreadyProcessedTheStartingValue = true; + } else { + // We're traversing the starting value's children (that is: values at greater depths). We need to + // advance the cursor by calling next(). + type = reader.next(); + } - if (reader.isNullValue()) { - this.writeNull(type); - } - else { - switch (type) { - case NULL: - writeNull(); - if (_debug_on) System.out.print("-"); - break; - case BOOL: - writeBool(reader.booleanValue()); - if (_debug_on) System.out.print("b"); - break; - case INT: - writeInt(reader.bigIntegerValue()); - if (_debug_on) System.out.print("i"); - break; - case FLOAT: - writeFloat(reader.doubleValue()); - if (_debug_on) System.out.print("f"); - break; - case DECIMAL: - writeDecimal(reader.decimalValue()); - if (_debug_on) System.out.print("d"); - break; - case TIMESTAMP: - writeTimestamp(reader.timestampValue()); - if (_debug_on) System.out.print("t"); - break; - case STRING: - writeString(reader.stringValue()); - if (_debug_on) System.out.print("$"); - break; - case SYMBOL: - writeSymbolToken(reader.symbolValue()); - if (_debug_on) System.out.print("y"); - break; - case BLOB: - writeBlob(reader.newBytes()); - if (_debug_on) System.out.print("B"); - break; - case CLOB: - writeClob(reader.newBytes()); - if (_debug_on) System.out.print("L"); - break; - case STRUCT: - if (_debug_on) System.out.print("{"); - writeContainerRecursively(IonType.STRUCT, reader); - if (_debug_on) System.out.print("}"); - break; - case LIST: - if (_debug_on) System.out.print("["); - writeContainerRecursively(IonType.LIST, reader); - if (_debug_on) System.out.print("]"); - break; - case SEXP: - if (_debug_on) System.out.print("("); - writeContainerRecursively(IonType.SEXP, reader); - if (_debug_on) System.out.print(")"); - break; - default: - throw new IllegalStateException("Unknown value type: " + type); + if (type == null) { + // There are no more values at this level. If we're at the starting level, we're done. + if (getDepth() == startingDepth) { + break; + } + // Otherwise, step out once and then try to move forward again. + reader.stepOut(); + stepOut(); + continue; } - } - } - private void writeContainerRecursively(IonType type, IonReader reader) - throws IOException - { - stepIn(type); - reader.stepIn(); - while ((type = reader.next()) != null) - { - writeValueRecursively(type, reader); + // We found a value. Write out its field ID and annotations, if any. + write_value_field_name_helper(reader); + write_value_annotations_helper(reader); + + if (reader.isNullValue()) { + this.writeNull(type); + continue; + } + + 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: + reader.stepIn(); + stepIn(IonType.STRUCT); + break; + case LIST: + reader.stepIn(); + stepIn(IonType.LIST); + break; + case SEXP: + reader.stepIn(); + stepIn(IonType.SEXP); + break; + default: + throw new IllegalStateException("Unknown value type: " + type); + } } - reader.stepOut(); - stepOut(); } - // // This code handles the skipped symbol table // support - it is cloned in IonReaderTextUserX, From 7a9c732823128bbd2593917f5dd1eeeac22eea40 Mon Sep 17 00:00:00 2001 From: Zack Slayton Date: Wed, 16 Jun 2021 16:27:32 -0400 Subject: [PATCH 2/3] Tweaked javadoc, removed parameter. --- .../amazon/ion/impl/IonWriterUserBinary.java | 2 +- .../ion/impl/_Private_IonWriterBase.java | 25 +++++++------------ 2 files changed, 10 insertions(+), 17 deletions(-) diff --git a/src/com/amazon/ion/impl/IonWriterUserBinary.java b/src/com/amazon/ion/impl/IonWriterUserBinary.java index 8d9a7823dd..ae1c072f3b 100644 --- a/src/com/amazon/ion/impl/IonWriterUserBinary.java +++ b/src/com/amazon/ion/impl/IonWriterUserBinary.java @@ -105,7 +105,7 @@ public void writeValue(IonReader reader) // From here on, we won't call back into this method, so we won't // bother doing all those checks again. - writeValueRecursively(type, reader); + writeValueRecursively(reader); } diff --git a/src/com/amazon/ion/impl/_Private_IonWriterBase.java b/src/com/amazon/ion/impl/_Private_IonWriterBase.java index 957d662296..f44a46add6 100644 --- a/src/com/amazon/ion/impl/_Private_IonWriterBase.java +++ b/src/com/amazon/ion/impl/_Private_IonWriterBase.java @@ -349,8 +349,7 @@ public boolean isStreamCopyOptimized() public void writeValue(IonReader reader) throws IOException { // TODO this should do symtab optimization as per writeValues() - IonType type = reader.getType(); - writeValueRecursively(type, reader); + writeValueRecursively(reader); } /** @@ -367,11 +366,11 @@ public void writeValue(IonReader reader) throws IOException * - If the writer is in a struct but the IonReader is not, this function throws an IllegalStateException. * * @param reader The IonReader that will provide a value to write. - * @throws IOException if the provided IonReader throws an IOException. + * @throws IOException if either the provided IonReader or this writer's underlying OutputStream throw an + * IOException. * @throws IllegalStateException if this writer is inside a struct but the IonReader is not. */ - final void writeValueRecursively(IonType ionType, IonReader reader) - throws IOException + final void writeValueRecursively(IonReader reader) throws IOException { // The IonReader does not need to be at the top level (getDepth()==0) when the function is called. // We take note of its initial depth so we can avoid advancing the IonReader beyond the starting value. @@ -398,7 +397,7 @@ final void writeValueRecursively(IonType ionType, IonReader reader) } // We're beginning our traversal. Don't advance the cursor; instead, use the IonType that was // passed in by the caller. - type = ionType; + type = reader.getType(); // We've begun processing the starting value. alreadyProcessedTheStartingValue = true; } else { @@ -418,7 +417,7 @@ final void writeValueRecursively(IonType ionType, IonReader reader) continue; } - // We found a value. Write out its field ID and annotations, if any. + // We found a value. Write out its field name and annotations, if any. write_value_field_name_helper(reader); write_value_annotations_helper(reader); @@ -458,17 +457,11 @@ final void writeValueRecursively(IonType ionType, IonReader reader) case CLOB: writeClob(reader.newBytes()); break; - case STRUCT: - reader.stepIn(); - stepIn(IonType.STRUCT); - break; - case LIST: - reader.stepIn(); - stepIn(IonType.LIST); - break; + case STRUCT: // Intentional fallthrough + case LIST: // Intentional fallthrough case SEXP: reader.stepIn(); - stepIn(IonType.SEXP); + stepIn(type); break; default: throw new IllegalStateException("Unknown value type: " + type); From aaff53ad6b3fe836b2284f94df766c6d89d8fdea Mon Sep 17 00:00:00 2001 From: Zack Slayton Date: Wed, 16 Jun 2021 16:31:39 -0400 Subject: [PATCH 3/3] Fixed comment. --- src/com/amazon/ion/impl/_Private_IonWriterBase.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/com/amazon/ion/impl/_Private_IonWriterBase.java b/src/com/amazon/ion/impl/_Private_IonWriterBase.java index f44a46add6..1aa806ec67 100644 --- a/src/com/amazon/ion/impl/_Private_IonWriterBase.java +++ b/src/com/amazon/ion/impl/_Private_IonWriterBase.java @@ -395,8 +395,8 @@ final void writeValueRecursively(IonReader reader) throws IOException // We're finishing our traversal. break; } - // We're beginning our traversal. Don't advance the cursor; instead, use the IonType that was - // passed in by the caller. + // We're beginning our traversal. Don't advance the cursor; instead, use the current + // value's IonType. type = reader.getType(); // We've begun processing the starting value. alreadyProcessedTheStartingValue = true;