Only initiate PatchPoint when needed. (#565)

src/com/amazon/ion/impl/_Private_RecyclingQueue.java

@@ -20,6 +20,14 @@ public interface ElementFactory<T> {
         T newElement();
     }
 
+    @FunctionalInterface
+    public interface Recycler<T> {
+        /**
+         * Re-initialize an element
+         */
+        void recycle(T t);
+    }
+
     /**
      * Iterator for the queue.
      */
@@ -67,15 +75,16 @@ public T get(int index) {
      * 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 T push() {
+    public int push(Recycler<T> recycler) {
         currentIndex++;
         if (currentIndex >= elements.size()) {
             top = elementFactory.newElement();
             elements.add(top);
         }  else {
             top = elements.get(currentIndex);
         }
-        return top;
+        recycler.recycle(top);
+        return currentIndex;
     }
 
     /**

src/com/amazon/ion/impl/_Private_RecyclingStack.java

@@ -2,13 +2,25 @@
 
 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 <T> the type of elements stored.
  */
-public final class _Private_RecyclingStack<T> {
+public final class _Private_RecyclingStack<T> implements Iterable<T> {
+    private $Iterator stackIterator;
+    @Override
+    public ListIterator<T> iterator() {
+        if (stackIterator != null) {
+            stackIterator.cursor = _Private_RecyclingStack.this.currentIndex;
+        } else {
+            stackIterator = new $Iterator();
+        }
+        return stackIterator;
+    }
 
     /**
      * Factory for new stack elements.
@@ -22,18 +34,26 @@ public interface ElementFactory<T> {
         T newElement();
     }
 
+    @FunctionalInterface
+    public interface Recycler<T> {
+        /**
+         * Re-initialize an element
+         */
+        void recycle(T t);
+    }
+
     private final List<T> elements;
     private final ElementFactory<T> 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 stack has
+     * @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<T> elementFactory) {
-        elements = new ArrayList<T>(initialCapacity);
+        elements = new ArrayList<>(initialCapacity);
         this.elementFactory = elementFactory;
         currentIndex = -1;
         top = null;
@@ -44,14 +64,15 @@ public _Private_RecyclingStack(int initialCapacity, ElementFactory<T> elementFac
      * 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() {
+    public T push(Recycler<T> recycler) {
         currentIndex++;
         if (currentIndex >= elements.size()) {
             top = elementFactory.newElement();
             elements.add(top);
         }  else {
             top = elements.get(currentIndex);
         }
+        recycler.recycle(top);
         return top;
     }
 
@@ -92,4 +113,62 @@ public boolean isEmpty() {
     public int size() {
         return currentIndex + 1;
     }
+
+    private class $Iterator implements ListIterator<T> {
+        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();
+        }
+    }
+
 }

src/com/amazon/ion/impl/bin/IonRawBinaryWriter.java

@@ -55,10 +55,8 @@
 import java.io.OutputStream;
 import java.math.BigDecimal;
 import java.math.BigInteger;
-import java.util.ArrayList;
 import java.util.Iterator;
-import java.util.List;
-import java.util.NoSuchElementException;
+import java.util.ListIterator;
 
 /**
  * Low-level binary {@link IonWriter} that understands encoding concerns but doesn't operate with any sense of symbol table management.
@@ -258,37 +256,42 @@ private class ContainerInfo
         /** The size of the current value. */
         public long length;
         /**
-         * The index of the patch point placeholder.
+         * The index of the patch point if present, <tt>-1</tt> otherwise.
          */
-        public int placeholderPatchIndex;
+        public int patchIndex;
 
         public ContainerInfo()
         {
             type = null;
             position = -1;
             length = -1;
-            placeholderPatchIndex = -1;
+            patchIndex = -1;
         }
 
         public void appendPatch(final long oldPosition, final int oldLength, final long length)
         {
-            patchPoints.get(placeholderPatchIndex).initialize(oldPosition, oldLength, 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);
+            }
         }
 
-        public void initialize(final ContainerType type, final long offset) {
+        public ContainerInfo initialize(final ContainerType type, final long offset) {
             this.type = type;
             this.position = offset;
             this.length = 0;
-            if (type != ContainerType.VALUE) {
-                placeholderPatchIndex = patchPoints.size();
-                patchPoints.push().initialize(-1, -1, -1);
-            }
+            this.patchIndex = -1;
+
+            return this;
         }
 
         @Override
         public String toString()
         {
-            return "(CI " + type + " pos:" + position + " len:" + length + ")";
+            return "(CI " + type + " pos:" + position + " len:" + length + " patch:"+patchIndex+")";
         }
     }
 
@@ -313,10 +316,15 @@ public String toString()
             return "(PP old::(" + oldPosition + " " + oldLength + ") patch::(" + length + ")";
         }
 
-        public void initialize(final long oldPosition, final int oldLength, final long length) {
+        public PatchPoint initialize(final long oldPosition, final int oldLength, final long length) {
             this.oldPosition = oldPosition;
             this.oldLength = oldLength;
             this.length = length;
+            return this;
+        }
+
+        public PatchPoint clear() {
+            return initialize(-1, -1, -1);
         }
     }
 
@@ -391,7 +399,6 @@ public ContainerInfo newElement() {
         this.currentFieldSid                  = SID_UNASSIGNED;
         this.currentAnnotationSids            = new IntList();
         this.hasTopLevelSymbolTableAnnotation = false;
-
         this.closed = false;
     }
 
@@ -440,6 +447,7 @@ public void setTypeAnnotationSymbols(final SymbolToken... annotations)
         {
             for (final SymbolToken annotation : annotations)
             {
+                if (annotation == null) break;
                 addTypeAnnotationSymbol(annotation.getSid());
             }
         }
@@ -543,23 +551,30 @@ private void updateLength(long length)
     private void pushContainer(final ContainerType type)
     {
         // XXX we push before writing the type of container
-        containers.push().initialize(type, buffer.position() + 1);
+        containers.push(c -> c.initialize(type, buffer.position() + 1));
     }
 
-    private void addPatchPoint(final long position, final int oldLength, final long value, final ContainerInfo container)
+    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<ContainerInfo> 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);
+            }
+        }
+
         // record the size of the length data.
         final int patchLength = WriteBuffer.varUIntLength(value);
-        if (container == null)
-        {
-            // not nested, just append to the root list
-            patchPoints.push().initialize(position, oldLength, value);
-        }
-        else
-        {
-            // nested, apply it to the current container
-            container.appendPatch(position, oldLength, value);
-        }
+        container.appendPatch(position, oldLength, value);
         updateLength(patchLength - oldLength);
     }
 
@@ -613,22 +628,20 @@ private ContainerInfo popContainer()
 
                 // We've reclaimed some number of bytes; adjust the container length as appropriate.
                 length -= numberOfBytesToShiftBy;
-                reclaimPlaceholderPatchPoint(currentContainer.placeholderPatchIndex);
             }
             else if (currentContainer.length <= preallocationMode.contentMaxLength)
             {
                 // The container's encoded body is too long to fit the length in the type descriptor byte, but it will
                 // fit in the preallocated length bytes that were added to the buffer when the container was started.
                 // Update those bytes with the VarUInt encoding of the length value.
                 preallocationMode.patchLength(buffer, positionOfFirstLengthByte, length);
-                reclaimPlaceholderPatchPoint(currentContainer.placeholderPatchIndex);
             }
             else
             {
                 // The container's encoded body is too long to fit in the length bytes that were preallocated.
-                // Write the VarUInt encoding of the length in a secondary buffer and make a note to include that
-                // when we go to flush the primary buffer to the output stream.
-                addPatchPoint(positionOfFirstLengthByte, preallocationMode.numberOfLengthBytes(), length, currentContainer);
+                // Write the length in the patch point list, making a note to include the VarUInt encoding of the length
+                // at the right point when we go to flush the primary buffer to the output stream.
+                addPatchPoint(currentContainer, positionOfFirstLengthByte, preallocationMode.numberOfLengthBytes(), length);
             }
         }
 
@@ -1074,7 +1087,7 @@ private void patchSingleByteTypedOptimisticValue(final byte type, final Containe
         {
             // side patch
             buffer.writeUInt8At(info.position - 1, type | 0xE);
-            addPatchPoint(info.position, 0, info.length, null);
+            addPatchPoint(info, info.position, 0, info.length);
         }
     }
 

src/com/amazon/ion/impl/bin/WriteBuffer.java

@@ -1070,7 +1070,7 @@ public static int varUIntLength(final long value)
         {
             return 8;
         }
-        throw new IllegalStateException("ion-java failed to support your use case.");
+        return 9;
     }
 
     /** Write the varUint value to the outputStream. */