From 0e835c6fd3d629f03b5c09c37bdce682943fde8d Mon Sep 17 00:00:00 2001 From: austnwil Date: Tue, 30 Sep 2025 20:37:59 -0700 Subject: [PATCH 01/13] Remove RecyclingStack - manage container stack directly in writer --- .../ion/impl/bin/IonRawBinaryWriter.java | 69 ++++++++++--------- 1 file changed, 36 insertions(+), 33 deletions(-) diff --git a/src/main/java/com/amazon/ion/impl/bin/IonRawBinaryWriter.java b/src/main/java/com/amazon/ion/impl/bin/IonRawBinaryWriter.java index e2073126c..99da022d8 100644 --- a/src/main/java/com/amazon/ion/impl/bin/IonRawBinaryWriter.java +++ b/src/main/java/com/amazon/ion/impl/bin/IonRawBinaryWriter.java @@ -34,7 +34,6 @@ import com.amazon.ion.SymbolToken; import com.amazon.ion.Timestamp; import com.amazon.ion.impl._Private_RecyclingQueue; -import com.amazon.ion.impl._Private_RecyclingStack; import com.amazon.ion.impl.bin.utf8.Utf8StringEncoder; import com.amazon.ion.impl.bin.utf8.Utf8StringEncoderPool; @@ -42,8 +41,8 @@ import java.io.OutputStream; import java.math.BigDecimal; import java.math.BigInteger; +import java.util.ArrayList; import java.util.Iterator; -import java.util.ListIterator; /** * Low-level binary {@link IonWriter} that understands encoding concerns but doesn't operate with any sense of symbol table management. @@ -333,7 +332,9 @@ public PatchPoint clear() { private final boolean isFloatBinary32Enabled; private final WriteBuffer buffer; private final _Private_RecyclingQueue patchPoints; - private final _Private_RecyclingStack containers; + private final ArrayList containers; + private int containerIndex; + private ContainerInfo topContainer; private int depth; private boolean hasWrittenValuesSinceFinished; private boolean hasWrittenValuesSinceConstructed; @@ -381,14 +382,9 @@ interface ThrowingRunnable { this.isFloatBinary32Enabled = isFloatBinary32Enabled; this.buffer = new WriteBuffer(allocator, this::endOfBlockSizeReached); this.patchPoints = new _Private_RecyclingQueue<>(512, PatchPoint::new); - this.containers = new _Private_RecyclingStack( - 10, - new _Private_RecyclingStack.ElementFactory() { - public ContainerInfo newElement() { - return new ContainerInfo(); - } - } - ); + this.containers = new ArrayList<>(10); + this.containerIndex = -1; + topContainer = null; this.depth = 0; this.hasWrittenValuesSinceFinished = false; this.hasWrittenValuesSinceConstructed = false; @@ -545,39 +541,44 @@ public int getDepth() private void updateLength(long length) { - if (containers.isEmpty()) + if (containerIndex < 0) { return; } - containers.peek().length += length; + topContainer.length += length; } private void pushContainer(final ContainerType type) { // XXX we push before writing the type of container - containers.push(c -> c.initialize(type, buffer.position() + 1)); + containerIndex++; + final ContainerInfo existingElement = containerIndex > -1 && containerIndex < containers.size() ? containers.get(containerIndex) : null; + if (existingElement != null) { + existingElement.initialize(type, buffer.position() + 1); + topContainer = existingElement; + } else { + topContainer = new ContainerInfo().initialize(type, buffer.position() + 1); + containers.add(topContainer); + } } private void addPatchPoint(final ContainerInfo container, final long position, final int oldLength, final long value) { - // If we're adding a patch point we first need to ensure that all of our ancestors (containing values) already - // have a patch point. No container can be smaller than the contents, so all outer layers also require patches. - // Instead of allocating iterator, we share one iterator instance within the scope of the container stack and reset the cursor every time we track back to the ancestors. - ListIterator stackIterator = containers.iterator(); - // Walk down the stack until we find an ancestor which already has a patch point - while (stackIterator.hasNext() && stackIterator.next().patchIndex == -1); - - // The iterator cursor is now positioned on an ancestor container that has a patch point - // Ascend back up the stack, fixing the ancestors which need a patch point assigned before us - while (stackIterator.hasPrevious()) { - ContainerInfo ancestor = stackIterator.previous(); - if (ancestor.patchIndex == -1) { - ancestor.patchIndex = patchPoints.push(PatchPoint::clear); + if(containerIndex >= 0) { + int index; + for(index = containerIndex; index >= 0; index--) { + if(containers.get(index).patchIndex != -1) { + break; + } + } + for(int i = index + 1; i <= containerIndex; i++) { + containers.get(i).patchIndex = patchPoints.push(PatchPoint::clear); } } - // record the size of the length data. + + final int patchLength = WriteBuffer.varUIntLength(value); container.appendPatch(position, oldLength, value); updateLength(patchLength - oldLength); @@ -585,7 +586,9 @@ private void addPatchPoint(final ContainerInfo container, final long position, f private ContainerInfo popContainer() { - final ContainerInfo currentContainer = containers.pop(); + final ContainerInfo currentContainer = topContainer; + containerIndex--; + topContainer = containerIndex > -1 ? containers.get(containerIndex) : null; if (currentContainer == null) { throw new IllegalStateException("Tried to pop container state without said container"); @@ -718,7 +721,7 @@ private void prepareValue() /** Closes out annotations. */ private void finishValue() throws IOException { - if (!containers.isEmpty() && containers.peek().type == ContainerType.ANNOTATION) + if (containerIndex > -1 && topContainer.type == ContainerType.ANNOTATION) { // close out and patch the length popContainer(); @@ -756,7 +759,7 @@ public void stepOut() throws IOException { throw new IonException("Cannot step out with field name set"); } - if (containers.isEmpty() || !containers.peek().type.allowedInStepOut) + if (containerIndex < 0 || !topContainer.type.allowedInStepOut) { throw new IonException("Cannot step out when not in container"); } @@ -769,7 +772,7 @@ public void stepOut() throws IOException public boolean isInStruct() { - return !containers.isEmpty() && containers.peek().type == ContainerType.STRUCT; + return containerIndex > -1 && topContainer.type == ContainerType.STRUCT; } // Write Value Methods @@ -1359,7 +1362,7 @@ public void finish() throws IOException { return; } - if (!containers.isEmpty() || depth > 0) + if (containerIndex > -1 || depth > 0) { throw new IllegalStateException("Cannot finish within container: " + containers); } From 35f4649656e3f6664f3e68a3ef03173aee431bc2 Mon Sep 17 00:00:00 2001 From: austnwil Date: Wed, 1 Oct 2025 11:19:06 -0700 Subject: [PATCH 02/13] Remove RecyclingQueue - manage PatchPoint list directly in writer --- .../ion/impl/bin/IonRawBinaryWriter.java | 63 ++++++++++++------- 1 file changed, 42 insertions(+), 21 deletions(-) diff --git a/src/main/java/com/amazon/ion/impl/bin/IonRawBinaryWriter.java b/src/main/java/com/amazon/ion/impl/bin/IonRawBinaryWriter.java index 99da022d8..2a8056cd6 100644 --- a/src/main/java/com/amazon/ion/impl/bin/IonRawBinaryWriter.java +++ b/src/main/java/com/amazon/ion/impl/bin/IonRawBinaryWriter.java @@ -254,10 +254,30 @@ public void appendPatch(final long oldPosition, final int oldLength, final long { if (patchIndex == -1) { // We have no assigned patch point, we need to make our own - patchIndex = patchPoints.push(p -> p.initialize(oldPosition, oldLength, length)); + patchPointsLength++; + patchIndex = patchPointsLength - 1; + if (patchIndex < patchPoints.size()) { + final PatchPoint existingPatchPoint = patchPoints.get(patchIndex); + if (existingPatchPoint != null) { + existingPatchPoint.initialize(oldPosition, oldLength, length); + } else { + patchPoints.set(patchIndex, new PatchPoint().initialize(oldPosition, oldLength, length)); + } + } else { + patchPoints.ensureCapacity(patchPointsLength); + for (int i = patchPoints.size(); i < patchPointsLength - 1; i++) { + patchPoints.add(null); + } + patchPoints.add(new PatchPoint().initialize(oldPosition, oldLength, length)); + } } else { // We have an assigned patch point already, but we need to overwrite it with the correct data - patchPoints.get(patchIndex).initialize(oldPosition, oldLength, length); + final PatchPoint patchPoint = patchPoints.get(patchIndex); + if (patchPoint != null) { + patchPoints.get(patchIndex).initialize(oldPosition, oldLength, length); + } else { + patchPoints.set(patchIndex, new PatchPoint().initialize(oldPosition, oldLength, length)); + } } } @@ -331,7 +351,8 @@ public PatchPoint clear() { private final PreallocationMode preallocationMode; private final boolean isFloatBinary32Enabled; private final WriteBuffer buffer; - private final _Private_RecyclingQueue patchPoints; + private final ArrayList patchPoints; + private int patchPointsLength; private final ArrayList containers; private int containerIndex; private ContainerInfo topContainer; @@ -381,7 +402,8 @@ interface ThrowingRunnable { this.preallocationMode = preallocationMode; this.isFloatBinary32Enabled = isFloatBinary32Enabled; this.buffer = new WriteBuffer(allocator, this::endOfBlockSizeReached); - this.patchPoints = new _Private_RecyclingQueue<>(512, PatchPoint::new); + this.patchPoints = new ArrayList<>(512); + this.patchPointsLength = 0; this.containers = new ArrayList<>(10); this.containerIndex = -1; topContainer = null; @@ -565,7 +587,7 @@ private void pushContainer(final ContainerType type) private void addPatchPoint(final ContainerInfo container, final long position, final int oldLength, final long value) { - if(containerIndex >= 0) { + if (containerIndex >= 0) { int index; for(index = containerIndex; index >= 0; index--) { if(containers.get(index).patchIndex != -1) { @@ -573,12 +595,15 @@ private void addPatchPoint(final ContainerInfo container, final long position, f } } for(int i = index + 1; i <= containerIndex; i++) { - containers.get(i).patchIndex = patchPoints.push(PatchPoint::clear); + // if (patchPointsLength < patchPoints.size()) { + // patchPoints.set(patchPointsLength, null); + // } else { + // patchPoints.add(null); + // } + containers.get(i).patchIndex = patchPointsLength++; } } - - final int patchLength = WriteBuffer.varUIntLength(value); container.appendPatch(position, oldLength, value); updateLength(patchLength - oldLength); @@ -1340,17 +1365,14 @@ public void writeBytes(byte[] data, int offset, int length) throws IOException { buffer.truncate(position); // TODO decide if it is worth making this faster than O(N) - Iterator patchIterator = patchPoints.iterate(); - int i = 0; - while (patchIterator.hasNext()) { - PatchPoint patchPoint = patchIterator.next(); - if (patchPoint.length > -1) { + for (int i = 0; i < patchPointsLength; i++) { + final PatchPoint patchPoint = patchPoints.get(i); + if (patchPoint != null && patchPoint.length > -1) { if (patchPoint.oldPosition >= position) { - patchPoints.truncate(i - 1); + patchPointsLength = i; break; } } - i++; } } @@ -1366,7 +1388,7 @@ public void finish() throws IOException { throw new IllegalStateException("Cannot finish within container: " + containers); } - if (patchPoints.isEmpty()) + if (patchPointsLength == 0) { // nothing to patch--write 'em out! buffer.writeTo(out); @@ -1374,11 +1396,10 @@ public void finish() throws IOException else { long bufferPosition = 0; - Iterator iterator = patchPoints.iterate(); - while (iterator.hasNext()) + for (int i = 0; i < patchPointsLength; i++) { - PatchPoint patch = iterator.next(); - if (patch.length < 0) { + final PatchPoint patch = patchPoints.get(i); + if (patch == null || patch.length < 0) { continue; } // write up to the thing to be patched @@ -1394,7 +1415,7 @@ public void finish() throws IOException } buffer.writeTo(out, bufferPosition, buffer.position() - bufferPosition); } - patchPoints.clear(); + patchPointsLength = 0; buffer.reset(); if (streamFlushMode == StreamFlushMode.FLUSH) From 83713158873bacb11c46854a71350e6b6a616f34 Mon Sep 17 00:00:00 2001 From: austnwil Date: Wed, 1 Oct 2025 12:54:35 -0700 Subject: [PATCH 03/13] Set version for local testing --- project.version | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/project.version b/project.version index 2d6981806..4096ffd0a 100644 --- a/project.version +++ b/project.version @@ -1 +1 @@ -1.11.11-SNAPSHOT +1.11.11-SNAPSHOT-no-container-or-patchpoint-stack From 8b2aa2e2529e5fec138c5a2f38c1e7a9331baa77 Mon Sep 17 00:00:00 2001 From: austnwil Date: Wed, 1 Oct 2025 16:58:51 -0700 Subject: [PATCH 04/13] Delete unused data structures and imports --- .../ion/impl/_Private_RecyclingQueue.java | 122 ------------ .../ion/impl/_Private_RecyclingStack.java | 174 ------------------ .../ion/impl/bin/IonRawBinaryWriter.java | 2 - 3 files changed, 298 deletions(-) delete mode 100644 src/main/java/com/amazon/ion/impl/_Private_RecyclingQueue.java delete mode 100644 src/main/java/com/amazon/ion/impl/_Private_RecyclingStack.java diff --git a/src/main/java/com/amazon/ion/impl/_Private_RecyclingQueue.java b/src/main/java/com/amazon/ion/impl/_Private_RecyclingQueue.java deleted file mode 100644 index a52ccd7a3..000000000 --- a/src/main/java/com/amazon/ion/impl/_Private_RecyclingQueue.java +++ /dev/null @@ -1,122 +0,0 @@ -package com.amazon.ion.impl; - -import java.util.ArrayList; -import java.util.Iterator; -import java.util.List; -/** - * A queue whose elements are recycled. This queue will be extended and iterated frequently. - * @param the type of elements stored. - */ -public class _Private_RecyclingQueue { - - /** - * Factory for new queue elements. - * @param the type of element. - */ - public interface ElementFactory { - /** - * @return a new instance. - */ - T newElement(); - } - - @FunctionalInterface - public interface Recycler { - /** - * Re-initialize an element - */ - void recycle(T t); - } - - /** - * Iterator for the queue. - */ - private class ElementIterator implements Iterator { - int i = 0; - @Override - public boolean hasNext() { - return i <= currentIndex; - } - - @Override - public T next() { - return elements.get(i++); - } - } - - private final ElementIterator iterator; - private final List elements; - private final ElementFactory elementFactory; - private int currentIndex; - private T top; - - /** - * @param initialCapacity the initial capacity of the underlying collection. - * @param elementFactory the factory used to create a new element on {@link #push()} when the queue has - * not previously grown to the new depth. - */ - public _Private_RecyclingQueue(int initialCapacity, ElementFactory elementFactory) { - elements = new ArrayList(initialCapacity); - this.elementFactory = elementFactory; - currentIndex = -1; - iterator = new ElementIterator(); - } - - public void truncate(int index) { - currentIndex = index; - } - - public T get(int index) { - return elements.get(index); - } - - /** - * Pushes an element onto the top of the queue, instantiating a new element only if the queue has not - * previously grown to the new depth. - * @return the element at the top of the queue after the push. This element must be initialized by the caller. - */ - public int push(Recycler recycler) { - currentIndex++; - if (currentIndex >= elements.size()) { - top = elementFactory.newElement(); - elements.add(top); - } else { - top = elements.get(currentIndex); - } - recycler.recycle(top); - return currentIndex; - } - - /** - * Reclaim the current element. - */ - public void remove() { - currentIndex = Math.max(-1, currentIndex - 1); - } - - public Iterator iterate() { - iterator.i = 0; - return iterator; - } - - /** - * @return true if the queue is empty; otherwise, false. - */ - public boolean isEmpty() { - return currentIndex < 0; - } - - /** - * Reset the index and make the queue reusable. - */ - public void clear() { - currentIndex = -1; - } - - /** - * @return the number of elements within the queue. - */ - public int size() { - return currentIndex + 1; - } -} \ No newline at end of file diff --git a/src/main/java/com/amazon/ion/impl/_Private_RecyclingStack.java b/src/main/java/com/amazon/ion/impl/_Private_RecyclingStack.java deleted file mode 100644 index d88ac040f..000000000 --- a/src/main/java/com/amazon/ion/impl/_Private_RecyclingStack.java +++ /dev/null @@ -1,174 +0,0 @@ -package com.amazon.ion.impl; - -import java.util.ArrayList; -import java.util.List; -import java.util.ListIterator; -import java.util.NoSuchElementException; - -/** - * A stack whose elements are recycled. This can be useful when the stack needs to grow and shrink - * frequently and has a predictable maximum depth. - * @param the type of elements stored. - */ -public final class _Private_RecyclingStack implements Iterable { - public $Iterator stackIterator; - @Override - public ListIterator iterator() { - if (stackIterator != null) { - stackIterator.cursor = _Private_RecyclingStack.this.currentIndex; - } else { - stackIterator = new $Iterator(); - } - return stackIterator; - } - - /** - * Factory for new stack elements. - * @param the type of element. - */ - public interface ElementFactory { - - /** - * @return a new instance. - */ - T newElement(); - } - - @FunctionalInterface - public interface Recycler { - /** - * Re-initialize an element - */ - void recycle(T t); - } - - private final List elements; - private final ElementFactory elementFactory; - private int currentIndex; - private T top; - - /** - * @param initialCapacity the initial capacity of the underlying collection. - * @param elementFactory the factory used to create a new element on {@link #push(Recycler)} when the stack has - * not previously grown to the new depth. - */ - public _Private_RecyclingStack(int initialCapacity, ElementFactory elementFactory) { - elements = new ArrayList<>(initialCapacity); - this.elementFactory = elementFactory; - currentIndex = -1; - top = null; - } - - /** - * Pushes an element onto the top of the stack, instantiating a new element only if the stack has not - * previously grown to the new depth. - * @return the element at the top of the stack after the push. This element must be initialized by the caller. - */ - public T push(Recycler recycler) { - currentIndex++; - if (currentIndex >= elements.size()) { - top = elementFactory.newElement(); - elements.add(top); - } else { - top = elements.get(currentIndex); - } - recycler.recycle(top); - return top; - } - - /** - * @return the element at the top of the stack, or null if the stack is empty. - */ - public T peek() { - return top; - } - - /** - * Pops an element from the stack, retaining a reference to the element so that it can be reused the - * next time the stack grows to the element's depth. - * @return the element that was at the top of the stack before the pop, or null if the stack was empty. - */ - public T pop() { - T popped = top; - currentIndex--; - if (currentIndex >= 0) { - top = elements.get(currentIndex); - } else { - top = null; - currentIndex = -1; - } - return popped; - } - - /** - * @return true if the stack is empty; otherwise, false. - */ - public boolean isEmpty() { - return top == null; - } - - /** - * @return the number of elements on the stack. - */ - public int size() { - return currentIndex + 1; - } - - private class $Iterator implements ListIterator { - private int cursor; - - @Override - public boolean hasNext() { - return cursor >= 0; - } - - @Override - public T next() { - if (!hasNext()) { - throw new NoSuchElementException(); - } - // post-decrement because "next" is where the cursor is - return _Private_RecyclingStack.this.elements.get(cursor--); - } - - @Override - public boolean hasPrevious() { - return cursor + 1 <= _Private_RecyclingStack.this.currentIndex; - } - - @Override - public T previous() { - if (!hasPrevious()) { - throw new NoSuchElementException(); - } - // pre-increment: "next" is where the cursor is, so "previous" is upward in stack - return _Private_RecyclingStack.this.elements.get(++cursor); - } - - @Override - public int nextIndex() { - return cursor; - } - - @Override - public int previousIndex() { - return cursor + 1; - } - - @Override - public void remove() { - throw new UnsupportedOperationException(); - } - - @Override - public void set(T t) { - throw new UnsupportedOperationException(); - } - - @Override - public void add(T t) { - throw new UnsupportedOperationException(); - } - } - -} diff --git a/src/main/java/com/amazon/ion/impl/bin/IonRawBinaryWriter.java b/src/main/java/com/amazon/ion/impl/bin/IonRawBinaryWriter.java index 2a8056cd6..12262c3f2 100644 --- a/src/main/java/com/amazon/ion/impl/bin/IonRawBinaryWriter.java +++ b/src/main/java/com/amazon/ion/impl/bin/IonRawBinaryWriter.java @@ -33,7 +33,6 @@ import com.amazon.ion.SymbolTable; import com.amazon.ion.SymbolToken; import com.amazon.ion.Timestamp; -import com.amazon.ion.impl._Private_RecyclingQueue; import com.amazon.ion.impl.bin.utf8.Utf8StringEncoder; import com.amazon.ion.impl.bin.utf8.Utf8StringEncoderPool; @@ -42,7 +41,6 @@ import java.math.BigDecimal; import java.math.BigInteger; import java.util.ArrayList; -import java.util.Iterator; /** * Low-level binary {@link IonWriter} that understands encoding concerns but doesn't operate with any sense of symbol table management. From 2afe8746e2667ac26abe31623d738942a0dc62bf Mon Sep 17 00:00:00 2001 From: austnwil Date: Wed, 1 Oct 2025 18:51:38 -0700 Subject: [PATCH 05/13] Code cleanup --- .../ion/impl/bin/IonRawBinaryWriter.java | 40 +++++++++++++------ 1 file changed, 27 insertions(+), 13 deletions(-) diff --git a/src/main/java/com/amazon/ion/impl/bin/IonRawBinaryWriter.java b/src/main/java/com/amazon/ion/impl/bin/IonRawBinaryWriter.java index 12262c3f2..fb1564b11 100644 --- a/src/main/java/com/amazon/ion/impl/bin/IonRawBinaryWriter.java +++ b/src/main/java/com/amazon/ion/impl/bin/IonRawBinaryWriter.java @@ -248,6 +248,26 @@ public ContainerInfo() patchIndex = -1; } + /** + * Set the data in the patch point assigned to this container. + * + * PatchPoint instances in {@link #patchPoints} are reused across the lifecycle of the writer, so + * there is a chance that a patch point already exists at {@link #patchIndex} with stale data + * that needs to be replaced. However, PatchPoints are not constructed until their first use, + * meaning the slot pointed to by {@link #patchIndex} may be null, in which case we have to + * create our own. This can occur if a child container ends up needing a patch point and some + * of its ancestors are missing patch points; the parents are assigned indices into {@link #patchPoints} + * but the patch point object itself was not constructed until now. + */ + private void setPatchPointData(final long oldPosition, final int oldLength, final long length) { + final PatchPoint existingPatchPoint = patchPoints.get(patchIndex); + if (existingPatchPoint != null) { + existingPatchPoint.initialize(oldPosition, oldLength, length); + } else { + patchPoints.set(patchIndex, new PatchPoint().initialize(oldPosition, oldLength, length)); + } + } + public void appendPatch(final long oldPosition, final int oldLength, final long length) { if (patchIndex == -1) { @@ -255,13 +275,12 @@ public void appendPatch(final long oldPosition, final int oldLength, final long patchPointsLength++; patchIndex = patchPointsLength - 1; if (patchIndex < patchPoints.size()) { - final PatchPoint existingPatchPoint = patchPoints.get(patchIndex); - if (existingPatchPoint != null) { - existingPatchPoint.initialize(oldPosition, oldLength, length); - } else { - patchPoints.set(patchIndex, new PatchPoint().initialize(oldPosition, oldLength, length)); - } + // The patch point ArrayList is large enough, the index of this patch point is in bounds + setPatchPointData(oldPosition, oldLength, length); } else { + // The patch point ArrayList is not large enough. It needs to grow to accomodate this + // patch point. No need to call setPatchPointData since we know the PatchPoint instance + // is missing. patchPoints.ensureCapacity(patchPointsLength); for (int i = patchPoints.size(); i < patchPointsLength - 1; i++) { patchPoints.add(null); @@ -270,12 +289,7 @@ public void appendPatch(final long oldPosition, final int oldLength, final long } } else { // We have an assigned patch point already, but we need to overwrite it with the correct data - final PatchPoint patchPoint = patchPoints.get(patchIndex); - if (patchPoint != null) { - patchPoints.get(patchIndex).initialize(oldPosition, oldLength, length); - } else { - patchPoints.set(patchIndex, new PatchPoint().initialize(oldPosition, oldLength, length)); - } + setPatchPointData(oldPosition, oldLength, length); } } @@ -404,7 +418,7 @@ interface ThrowingRunnable { this.patchPointsLength = 0; this.containers = new ArrayList<>(10); this.containerIndex = -1; - topContainer = null; + this.topContainer = null; this.depth = 0; this.hasWrittenValuesSinceFinished = false; this.hasWrittenValuesSinceConstructed = false; From 933f00b8761bd970deabc1f687318a9a898528cf Mon Sep 17 00:00:00 2001 From: austnwil Date: Thu, 2 Oct 2025 10:34:23 -0700 Subject: [PATCH 06/13] Add code documentation --- .../ion/impl/bin/IonRawBinaryWriter.java | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/src/main/java/com/amazon/ion/impl/bin/IonRawBinaryWriter.java b/src/main/java/com/amazon/ion/impl/bin/IonRawBinaryWriter.java index fb1564b11..74cf87840 100644 --- a/src/main/java/com/amazon/ion/impl/bin/IonRawBinaryWriter.java +++ b/src/main/java/com/amazon/ion/impl/bin/IonRawBinaryWriter.java @@ -275,7 +275,8 @@ public void appendPatch(final long oldPosition, final int oldLength, final long patchPointsLength++; patchIndex = patchPointsLength - 1; if (patchIndex < patchPoints.size()) { - // The patch point ArrayList is large enough, the index of this patch point is in bounds + // The patch point ArrayList is large enough, the index of this patch point is in bounds. + // A stale reusable patch point instance may be waiting for us here, or this may be a null slot. setPatchPointData(oldPosition, oldLength, length); } else { // The patch point ArrayList is not large enough. It needs to grow to accomodate this @@ -364,9 +365,14 @@ public PatchPoint clear() { private final boolean isFloatBinary32Enabled; private final WriteBuffer buffer; private final ArrayList patchPoints; + /** The length of the patch point queue. Some elements in the queue may be null or not yet have the correct data. */ private int patchPointsLength; private final ArrayList containers; + /** + * The index in {@link #containers} of the element at the top of the stack, or -1 if the stack is empty. + * Always one less than the length of the stack itself. */ private int containerIndex; + /** The container at the top of the container stack, or null if the container stack is empty */ private ContainerInfo topContainer; private int depth; private boolean hasWrittenValuesSinceFinished; @@ -599,23 +605,23 @@ private void pushContainer(final ContainerType type) private void addPatchPoint(final ContainerInfo container, final long position, final int oldLength, final long value) { + // If we're adding a patch point we first need to ensure that all of our ancestors (containing values) already + // have a patch point. No container can be smaller than the contents, so all outer layers also require patches. if (containerIndex >= 0) { int index; + // Walk down the stack until we find an ancestor which already has a patch point for(index = containerIndex; index >= 0; index--) { if(containers.get(index).patchIndex != -1) { break; } } + // index is now positioned on an ancestor container that has a patch point for(int i = index + 1; i <= containerIndex; i++) { - // if (patchPointsLength < patchPoints.size()) { - // patchPoints.set(patchPointsLength, null); - // } else { - // patchPoints.add(null); - // } containers.get(i).patchIndex = patchPointsLength++; } } + // record the size of the length data. final int patchLength = WriteBuffer.varUIntLength(value); container.appendPatch(position, oldLength, value); updateLength(patchLength - oldLength); From d7acc9606b084796e5ed371e5fb32145e138bb92 Mon Sep 17 00:00:00 2001 From: austnwil Date: Thu, 2 Oct 2025 10:34:38 -0700 Subject: [PATCH 07/13] Fix version --- project.version | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/project.version b/project.version index 4096ffd0a..2d6981806 100644 --- a/project.version +++ b/project.version @@ -1 +1 @@ -1.11.11-SNAPSHOT-no-container-or-patchpoint-stack +1.11.11-SNAPSHOT From 560b0ef6f1f94ace2317286a4a45b7b72e58c6b0 Mon Sep 17 00:00:00 2001 From: austnwil Date: Fri, 3 Oct 2025 10:13:34 -0700 Subject: [PATCH 08/13] Whitespace consistency fixes --- .../ion/impl/bin/IonRawBinaryWriter.java | 21 ++++++++++--------- 1 file changed, 11 insertions(+), 10 deletions(-) diff --git a/src/main/java/com/amazon/ion/impl/bin/IonRawBinaryWriter.java b/src/main/java/com/amazon/ion/impl/bin/IonRawBinaryWriter.java index 74cf87840..440e0ea31 100644 --- a/src/main/java/com/amazon/ion/impl/bin/IonRawBinaryWriter.java +++ b/src/main/java/com/amazon/ion/impl/bin/IonRawBinaryWriter.java @@ -259,7 +259,8 @@ public ContainerInfo() * of its ancestors are missing patch points; the parents are assigned indices into {@link #patchPoints} * but the patch point object itself was not constructed until now. */ - private void setPatchPointData(final long oldPosition, final int oldLength, final long length) { + private void setPatchPointData(final long oldPosition, final int oldLength, final long length) + { final PatchPoint existingPatchPoint = patchPoints.get(patchIndex); if (existingPatchPoint != null) { existingPatchPoint.initialize(oldPosition, oldLength, length); @@ -364,16 +365,16 @@ public PatchPoint clear() { private final PreallocationMode preallocationMode; private final boolean isFloatBinary32Enabled; private final WriteBuffer buffer; - private final ArrayList patchPoints; + private final ArrayList patchPoints; /** The length of the patch point queue. Some elements in the queue may be null or not yet have the correct data. */ - private int patchPointsLength; - private final ArrayList containers; + private int patchPointsLength; + private final ArrayList containers; /** * The index in {@link #containers} of the element at the top of the stack, or -1 if the stack is empty. * Always one less than the length of the stack itself. */ - private int containerIndex; + private int containerIndex; /** The container at the top of the container stack, or null if the container stack is empty */ - private ContainerInfo topContainer; + private ContainerInfo topContainer; private int depth; private boolean hasWrittenValuesSinceFinished; private boolean hasWrittenValuesSinceConstructed; @@ -610,13 +611,13 @@ private void addPatchPoint(final ContainerInfo container, final long position, f if (containerIndex >= 0) { int index; // Walk down the stack until we find an ancestor which already has a patch point - for(index = containerIndex; index >= 0; index--) { - if(containers.get(index).patchIndex != -1) { + for (index = containerIndex; index >= 0; index--) { + if (containers.get(index).patchIndex != -1) { break; } } // index is now positioned on an ancestor container that has a patch point - for(int i = index + 1; i <= containerIndex; i++) { + for (int i = index + 1; i <= containerIndex; i++) { containers.get(i).patchIndex = patchPointsLength++; } } @@ -992,7 +993,7 @@ public void writeInt(BigInteger value) throws IOException prepareValue(); int type = POS_INT_TYPE; - if(value.signum() < 0) + if (value.signum() < 0) { type = NEG_INT_TYPE; value = value.negate(); From e811137dc5c54112fc7387f95fe1f2eb6f326955 Mon Sep 17 00:00:00 2001 From: austnwil Date: Fri, 3 Oct 2025 11:05:00 -0700 Subject: [PATCH 09/13] Simplify container push logic --- .../com/amazon/ion/impl/bin/IonRawBinaryWriter.java | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/main/java/com/amazon/ion/impl/bin/IonRawBinaryWriter.java b/src/main/java/com/amazon/ion/impl/bin/IonRawBinaryWriter.java index 440e0ea31..e6195712f 100644 --- a/src/main/java/com/amazon/ion/impl/bin/IonRawBinaryWriter.java +++ b/src/main/java/com/amazon/ion/impl/bin/IonRawBinaryWriter.java @@ -594,11 +594,17 @@ private void pushContainer(final ContainerType type) { // XXX we push before writing the type of container containerIndex++; - final ContainerInfo existingElement = containerIndex > -1 && containerIndex < containers.size() ? containers.get(containerIndex) : null; - if (existingElement != null) { + if (containerIndex < containers.size()) { + // The container stack array is large enough, the index of this container is in bounds. + // A stale reusable container instance is waiting for us here to recycle. + // Note: the element at this container cannot be null. The only way for the container stack + // to grow is to push a container, and this always happens in order. Unlike patch points, we + // will never push a container at an index without its ancestors already being in the stack. + final ContainerInfo existingElement = containers.get(containerIndex); existingElement.initialize(type, buffer.position() + 1); topContainer = existingElement; } else { + // The container stack is not large enough. It needs to grow to accomodate this container. topContainer = new ContainerInfo().initialize(type, buffer.position() + 1); containers.add(topContainer); } From c831f0551d71a72efe7826ac8e86ebeb5da463dc Mon Sep 17 00:00:00 2001 From: austnwil Date: Fri, 3 Oct 2025 11:05:57 -0700 Subject: [PATCH 10/13] Delete unused function --- src/main/java/com/amazon/ion/impl/bin/IonRawBinaryWriter.java | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/main/java/com/amazon/ion/impl/bin/IonRawBinaryWriter.java b/src/main/java/com/amazon/ion/impl/bin/IonRawBinaryWriter.java index e6195712f..a89caef3f 100644 --- a/src/main/java/com/amazon/ion/impl/bin/IonRawBinaryWriter.java +++ b/src/main/java/com/amazon/ion/impl/bin/IonRawBinaryWriter.java @@ -338,10 +338,6 @@ public PatchPoint initialize(final long oldPosition, final int oldLength, final this.length = length; return this; } - - public PatchPoint clear() { - return initialize(-1, -1, -1); - } } /*package*/ enum StreamCloseMode From a87d59499278898561ca913b199c6cd9f08e72e2 Mon Sep 17 00:00:00 2001 From: Austin Williams Date: Wed, 8 Oct 2025 10:13:22 -0700 Subject: [PATCH 11/13] Apply suggestions from code review Co-authored-by: Joshua Barr <70981087+jobarr-amzn@users.noreply.github.com> --- .../amazon/ion/impl/bin/IonRawBinaryWriter.java | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-) diff --git a/src/main/java/com/amazon/ion/impl/bin/IonRawBinaryWriter.java b/src/main/java/com/amazon/ion/impl/bin/IonRawBinaryWriter.java index a89caef3f..8409f3fc2 100644 --- a/src/main/java/com/amazon/ion/impl/bin/IonRawBinaryWriter.java +++ b/src/main/java/com/amazon/ion/impl/bin/IonRawBinaryWriter.java @@ -273,22 +273,19 @@ public void appendPatch(final long oldPosition, final int oldLength, final long { if (patchIndex == -1) { // We have no assigned patch point, we need to make our own - patchPointsLength++; - patchIndex = patchPointsLength - 1; - if (patchIndex < patchPoints.size()) { - // The patch point ArrayList is large enough, the index of this patch point is in bounds. - // A stale reusable patch point instance may be waiting for us here, or this may be a null slot. - setPatchPointData(oldPosition, oldLength, length); - } else { + patchIndex = patchPointsLength++; + if (patchIndex >= patchPoints.size()) { // The patch point ArrayList is not large enough. It needs to grow to accomodate this // patch point. No need to call setPatchPointData since we know the PatchPoint instance // is missing. patchPoints.ensureCapacity(patchPointsLength); - for (int i = patchPoints.size(); i < patchPointsLength - 1; i++) { + for (int i = patchPoints.size(); i < patchPointsLength ; i++) { patchPoints.add(null); } - patchPoints.add(new PatchPoint().initialize(oldPosition, oldLength, length)); - } + } + + // A stale reusable patch point instance may be waiting for us here, or this may be a null slot. + setPatchPointData(oldPosition, oldLength, length); } else { // We have an assigned patch point already, but we need to overwrite it with the correct data setPatchPointData(oldPosition, oldLength, length); From 66a06bb5e73f46a4d62bc369a9eb428a3f422e96 Mon Sep 17 00:00:00 2001 From: austnwil Date: Wed, 8 Oct 2025 10:36:22 -0700 Subject: [PATCH 12/13] Comment adjustments --- .../com/amazon/ion/impl/bin/IonRawBinaryWriter.java | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/src/main/java/com/amazon/ion/impl/bin/IonRawBinaryWriter.java b/src/main/java/com/amazon/ion/impl/bin/IonRawBinaryWriter.java index 8409f3fc2..729469a46 100644 --- a/src/main/java/com/amazon/ion/impl/bin/IonRawBinaryWriter.java +++ b/src/main/java/com/amazon/ion/impl/bin/IonRawBinaryWriter.java @@ -275,11 +275,10 @@ public void appendPatch(final long oldPosition, final int oldLength, final long // We have no assigned patch point, we need to make our own patchIndex = patchPointsLength++; if (patchIndex >= patchPoints.size()) { - // The patch point ArrayList is not large enough. It needs to grow to accomodate this - // patch point. No need to call setPatchPointData since we know the PatchPoint instance - // is missing. + // The patch point ArrayList is not large enough. + // It needs to grow to accomodate this patch point. patchPoints.ensureCapacity(patchPointsLength); - for (int i = patchPoints.size(); i < patchPointsLength ; i++) { + for (int i = patchPoints.size(); i < patchPointsLength; i++) { patchPoints.add(null); } } @@ -414,8 +413,14 @@ interface ThrowingRunnable { this.preallocationMode = preallocationMode; this.isFloatBinary32Enabled = isFloatBinary32Enabled; this.buffer = new WriteBuffer(allocator, this::endOfBlockSizeReached); + // Patch point list initial capacity of 512 is fairly arbitrary and subject to tuning. + // When patch points are required, they are often required in bulk, since all ancestors of + // containers with large content require one. + // Some tuning has shown performance impacts - see https://github.com/amazon-ion/ion-java/issues/1095 this.patchPoints = new ArrayList<>(512); this.patchPointsLength = 0; + // Container stack initial capacity of 10 is fairly arbitrary. A max depth of + // 10 containers seems reasonable for common data. Subject to tuning. this.containers = new ArrayList<>(10); this.containerIndex = -1; this.topContainer = null; From afe9c7f19c1d67279fdd6eab1e6f749d37bc520c Mon Sep 17 00:00:00 2001 From: austnwil Date: Wed, 8 Oct 2025 10:49:08 -0700 Subject: [PATCH 13/13] Remove duplicate function call, unnecessary else --- .../com/amazon/ion/impl/bin/IonRawBinaryWriter.java | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/src/main/java/com/amazon/ion/impl/bin/IonRawBinaryWriter.java b/src/main/java/com/amazon/ion/impl/bin/IonRawBinaryWriter.java index 729469a46..f75a624d5 100644 --- a/src/main/java/com/amazon/ion/impl/bin/IonRawBinaryWriter.java +++ b/src/main/java/com/amazon/ion/impl/bin/IonRawBinaryWriter.java @@ -281,14 +281,12 @@ public void appendPatch(final long oldPosition, final int oldLength, final long for (int i = patchPoints.size(); i < patchPointsLength; i++) { patchPoints.add(null); } - } - - // A stale reusable patch point instance may be waiting for us here, or this may be a null slot. - setPatchPointData(oldPosition, oldLength, length); - } else { - // We have an assigned patch point already, but we need to overwrite it with the correct data - setPatchPointData(oldPosition, oldLength, length); + } } + + // A stale reusable patch point instance may be waiting for us here that we need to overwrite with + // the correct data, or this may be a null slot. If we just extended the ArrayList, then it is definitely null. + setPatchPointData(oldPosition, oldLength, length); } public ContainerInfo initialize(final ContainerType type, final long offset) {