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 e2073126c..f75a624d5 100644 --- a/src/main/java/com/amazon/ion/impl/bin/IonRawBinaryWriter.java +++ b/src/main/java/com/amazon/ion/impl/bin/IonRawBinaryWriter.java @@ -33,8 +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._Private_RecyclingStack; import com.amazon.ion.impl.bin.utf8.Utf8StringEncoder; import com.amazon.ion.impl.bin.utf8.Utf8StringEncoderPool; @@ -42,8 +40,7 @@ import java.io.OutputStream; import java.math.BigDecimal; import java.math.BigInteger; -import java.util.Iterator; -import java.util.ListIterator; +import java.util.ArrayList; /** * Low-level binary {@link IonWriter} that understands encoding concerns but doesn't operate with any sense of symbol table management. @@ -251,15 +248,45 @@ 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) { // We have no assigned patch point, we need to make our own - patchIndex = patchPoints.push(p -> p.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); + patchIndex = patchPointsLength++; + if (patchIndex >= patchPoints.size()) { + // 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++) { + patchPoints.add(null); + } + } } + + // 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) { @@ -305,10 +332,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 @@ -332,8 +355,16 @@ public PatchPoint clear() { private final PreallocationMode preallocationMode; private final boolean isFloatBinary32Enabled; private final WriteBuffer buffer; - private final _Private_RecyclingQueue patchPoints; - private final _Private_RecyclingStack containers; + 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; private boolean hasWrittenValuesSinceConstructed; @@ -380,15 +411,17 @@ interface ThrowingRunnable { this.preallocationMode = preallocationMode; 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(); - } - } - ); + // 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; this.depth = 0; this.hasWrittenValuesSinceFinished = false; this.hasWrittenValuesSinceConstructed = false; @@ -545,35 +578,49 @@ 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++; + 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); + } } 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; + // 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++) { + containers.get(i).patchIndex = patchPointsLength++; } } @@ -585,7 +632,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 +767,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 +805,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 +818,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 @@ -946,7 +995,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(); @@ -1337,17 +1386,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++; } } @@ -1359,11 +1405,11 @@ public void finish() throws IOException { return; } - if (!containers.isEmpty() || depth > 0) + if (containerIndex > -1 || depth > 0) { throw new IllegalStateException("Cannot finish within container: " + containers); } - if (patchPoints.isEmpty()) + if (patchPointsLength == 0) { // nothing to patch--write 'em out! buffer.writeTo(out); @@ -1371,11 +1417,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 @@ -1391,7 +1436,7 @@ public void finish() throws IOException } buffer.writeTo(out, bufferPosition, buffer.position() - bufferPosition); } - patchPoints.clear(); + patchPointsLength = 0; buffer.reset(); if (streamFlushMode == StreamFlushMode.FLUSH)