Adds support for add_symbols and set_symbols in BytecodeIonReader

Author: popematt

src/main/java/com/amazon/ion/bytecode/BytecodeIonReader.kt

@@ -10,6 +10,7 @@ import com.amazon.ion.IonType
 import com.amazon.ion.SymbolTable
 import com.amazon.ion.SymbolToken
 import com.amazon.ion.Timestamp
+import com.amazon.ion.UnknownSymbolException
 import com.amazon.ion.bytecode.BytecodeIonReader.AnnotationHelper.EMPTY_ANNOTATIONS
 import com.amazon.ion.bytecode.ir.Debugger
 import com.amazon.ion.bytecode.ir.Instructions
@@ -118,12 +119,7 @@ internal class BytecodeIonReader(private var generator: BytecodeGenerator) : Ion
 
         do {
             // Move `i` to point to the next instruction.
-            val length = Instructions.getData(instruction)
-            val operandCountBits = Instructions.getOperandCountBits(instruction)
-            // equivalent to `i += if (operandsToSkip == 3) length else operandsToSkip`
-            // `useOperandCount` is all zeros if `operandsToSkip` is 3, and all ones if `operandsToCount` is smaller than 3.
-            val useOperandCount = ((operandCountBits - 3) shr 2)
-            i += (operandCountBits and useOperandCount) or (length and useOperandCount.inv())
+            i += Instructions.getOperandCount(instruction)
 
             // Load the next instruction
             instruction = bytecode[i++]
@@ -202,8 +198,33 @@ internal class BytecodeIonReader(private var generator: BytecodeGenerator) : Ion
         context.reset()
     }
 
-    private fun handleSystemValue(instruction: Int, nextI: Int): Int {
-        TODO("Implement directive handler")
+    private fun handleSystemValue(instruction: Int, position: Int): Int {
+        val op = Instructions.toOperation(instruction)
+        this.instruction = INSTRUCTION_NOT_SET
+        bytecodeI = position
+
+        when (op) {
+            Operation.OP_DIRECTIVE_SET_SYMBOLS -> context.readSetSymbolsDirective(this)
+            Operation.OP_DIRECTIVE_ADD_SYMBOLS -> context.readAddSymbols(this)
+            Operation.OP_DIRECTIVE_SET_MACROS -> context.readSetMacrosDirective(this)
+            Operation.OP_DIRECTIVE_ADD_MACROS -> context.readAddMacrosDirective(this)
+
+            Operation.OP_DIRECTIVE_USE -> context.readUseDirective(this)
+            Operation.OP_DIRECTIVE_IMPORT -> context.readImportDirective(this)
+            Operation.OP_DIRECTIVE_ENCODING -> context.readEncodingDirective(this)
+            Operation.OP_DIRECTIVE_MODULE -> context.readModuleDirective(this)
+
+            else -> TODO()
+        }
+        // Ensure that we are positioned on/after the END_CONTAINER instruction.
+        bytecodeI += Instructions.getOperandCount(this.instruction) + 1
+        // Clear the current instruction, so that we can advance past the directive's CONTAINER_END
+        this.instruction = INSTRUCTION_NOT_SET
+
+        // This is required after any directive other than ADD/SET macros, so we'll just do this in all cases since it's a cheap operation.
+        symbolTable = context.getEffectiveSymbolTable()
+
+        return bytecodeI
     }
 
     override fun getType(): IonType? = OperationKind.ionTypeOf(Operation.toOperationKind(Instructions.toOperation(instruction)))
@@ -544,7 +565,10 @@ internal class BytecodeIonReader(private var generator: BytecodeGenerator) : Ion
             Operation.OP_SYMBOL_REF,
             Operation.OP_STRING_REF -> generator.readTextReference(position = bytecode[i], length = data)
             Operation.OP_SYMBOL_CHAR -> data.toChar().toString()
-            Operation.OP_SYMBOL_SID -> symbolTable[data]
+            Operation.OP_SYMBOL_SID -> {
+                // TODO: Check the size of the actual symbol table (not the `symbolTable` array, which may be over-allocated)
+                symbolTable[data]
+            }
             else -> throw IonException("Not positioned on a string or symbol value")
         }
     }
@@ -564,8 +588,7 @@ internal class BytecodeIonReader(private var generator: BytecodeGenerator) : Ion
         }
     }
 
-    // TODO: don't return null
-    override fun getSymbolTable(): SymbolTable? = null
+    override fun getSymbolTable(): SymbolTable = context.getLstSnapshot()
 
     override fun byteSize(): Int {
         val instruction = this.instruction

src/main/java/com/amazon/ion/bytecode/EncodingContextManager.kt

@@ -2,18 +2,29 @@
 // SPDX-License-Identifier: Apache-2.0
 package com.amazon.ion.bytecode
 
+import com.amazon.ion.IonException
+import com.amazon.ion.IonType
+import com.amazon.ion.SystemSymbols
 import com.amazon.ion.bytecode.util.BytecodeBuffer
 import com.amazon.ion.bytecode.util.ConstantPool
+import com.amazon.ion.bytecode.util.StringPool
+import com.amazon.ion.impl.ArrayBackedLstSnapshot
 import com.amazon.ion.ion_1_1.MacroImpl
 import edu.umd.cs.findbugs.annotations.SuppressFBWarnings
 
 /**
  * TODO:
  *   Write more documentation.
- *   Implement stubbed out methods.
+ *   Implement remaining stubbed out methods.
+ *   Do we need some way to "garbage collect" from the constant pool?
  *
  * Notes:
  *
+ * Terminology:
+ *  - "effective" symbol/macro table -- the tables that are currently in use and exposed to the reader
+ *  - "spare" symbol/macro table -- essentially we have an object pool with size 1, allowing us to modify the inactive
+ *    tables and then swap them for the active tables once the changes are complete.
+ *
  * It is never safe to remove or modify any existing data in the effective tables. It is safe to append data to those
  * tables for an `add_symbols`, `add_macros`, or `use` directive (as long as the active encoding modules are just `$ion` and `_`).
  */
@@ -34,14 +45,21 @@ internal class EncodingContextManager {
         )
     }
 
-    // These make up the effective macro table and effective symbol table
-    private var macroBytecode = BytecodeBuffer()
-    private var macroOffsets = BytecodeBuffer()
-    private var macroNames = ConstantPool()
-    private var symbols = mutableListOf<String?>().apply { SYSTEM_SYMBOLS.forEach { add(it) } }
-
-    // TODO: Do we need the constant pool here?
-    private var constants = ConstantPool()
+    class TableSet {
+        @JvmField var macroBytecode = BytecodeBuffer()
+        @JvmField var macroOffsets = BytecodeBuffer()
+        @JvmField var macroNames = ConstantPool()
+        @JvmField var symbols = StringPool().apply { SYSTEM_SYMBOLS.forEach { add(it) } }
+        @JvmField var constants = ConstantPool()
+
+        fun reset() {
+            macroBytecode.clear()
+            macroOffsets.clear()
+            macroNames.clear()
+            symbols.truncate(SystemSymbols.ION_1_0_MAX_ID + 1)
+            constants.clear()
+        }
+    }
 
     private class Module(
         val symbols: Array<String>,
@@ -54,34 +72,59 @@ internal class EncodingContextManager {
     // Tracks only modules _other_ than the system module and default module
     private var additionalActiveModules = mutableListOf<Module>()
 
-    @SuppressFBWarnings("IE_EXPOSE_REP", justification = "array is accessible for performance")
-    fun getEffectiveMacroTableBytecode(): IntArray = macroBytecode.unsafeGetArray()
+    private val effectiveTables = TableSet()
+    // TODO(simplification): we might not need the spare tables for macros because
+    //  macros are already evaluated before we get to this point.
+    private val spareTables = TableSet()
 
     @SuppressFBWarnings("IE_EXPOSE_REP", justification = "array is accessible for performance")
-    fun getEffectiveMacroTableOffsets(): IntArray = macroOffsets.unsafeGetArray()
-
-    fun getEffectiveSymbolTable(): Array<String?> = symbols.toTypedArray()
-
+    fun getEffectiveMacroTableBytecode(): IntArray = effectiveTables.macroBytecode.unsafeGetArray()
+    @SuppressFBWarnings("IE_EXPOSE_REP", justification = "array is accessible for performance")
+    fun getEffectiveMacroTableOffsets(): IntArray = effectiveTables.macroOffsets.unsafeGetArray()
+    @SuppressFBWarnings("IE_EXPOSE_REP", justification = "array is accessible for performance")
+    fun getEffectiveSymbolTable(): Array<String?> = effectiveTables.symbols.unsafeGetArray()
     @SuppressFBWarnings("IE_EXPOSE_REP", justification = "array is accessible for performance")
-    fun getEffectiveConstantPool(): Array<Any?> = constants.unsafeGetArray()
+    fun getEffectiveConstantPool(): Array<Any?> = effectiveTables.constants.unsafeGetArray()
+
+    fun getLstSnapshot() = ArrayBackedLstSnapshot(effectiveTables.symbols)
 
     /** Called when encountering an IVM */
     fun reset() {
         additionalActiveModules.clear()
         additionalAvailableModules.clear()
-        macroBytecode.clear()
-        macroOffsets.clear()
-        macroNames.clear()
-        symbols.clear()
-        SYSTEM_SYMBOLS.forEach { symbols.add(it) }
-        constants.clear()
+        effectiveTables.reset()
     }
 
     /**
      * The [BytecodeIonReader] should be positioned in the directive, but not on the first value yet.
      * When this method returns, the [BytecodeIonReader] will be positioned at the end of the directive, but not stepped out.
      */
     fun readSetSymbolsDirective(reader: BytecodeIonReader) {
+        if (additionalActiveModules.isNotEmpty()) {
+            readSetSymbolsWithActiveModules(reader)
+            return
+        }
+
+        val symbols = spareTables.symbols
+        symbols.truncate(SystemSymbols.ION_1_0_MAX_ID + 1)
+        while (true) {
+            val s = when (reader.next()) {
+                IonType.SYMBOL,
+                IonType.STRING -> reader.stringValue()
+                null -> break
+                else -> throw IonException("Expected text; found ${reader.type}")
+            }
+            symbols.add(s)
+        }
+
+        // Swap the effective and spare symbol tables
+        spareTables.symbols = effectiveTables.symbols
+        effectiveTables.symbols = symbols
+    }
+
+    private fun readSetSymbolsWithActiveModules(reader: BytecodeIonReader) {
+        // Update the default module then call:
+        // rebuildEffectiveSymbolTable(updateReaderSymbolTable)
         TODO()
     }
 
@@ -90,6 +133,26 @@ internal class EncodingContextManager {
      * When this method returns, the [BytecodeIonReader] will be positioned at the end of the directive, but not stepped out.
      */
     fun readAddSymbols(reader: BytecodeIonReader) {
+        if (additionalActiveModules.isNotEmpty()) {
+            readAddSymbolsWithActiveModules(reader)
+            return
+        }
+
+        val symbols = effectiveTables.symbols
+        while (true) {
+            val s = when (reader.next()) {
+                IonType.SYMBOL,
+                IonType.STRING -> reader.stringValue()
+                null -> break
+                else -> throw IonException("Expected text; found ${reader.type}")
+            }
+            symbols.add(s)
+        }
+    }
+
+    private fun readAddSymbolsWithActiveModules(reader: BytecodeIonReader) {
+        // Update the default module then call:
+        // rebuildEffectiveSymbolTable(updateReaderSymbolTable)
         TODO()
     }
 
@@ -142,4 +205,21 @@ internal class EncodingContextManager {
     fun readEncodingDirective(reader: BytecodeIonReader) {
         TODO()
     }
+
+    /**
+     * Rebuilds the effective symbol table using all the active encoding modules
+     */
+    private fun rebuildEffectiveSymbolTable() {
+        val newEffectiveSymbolTable = spareTables.symbols
+        newEffectiveSymbolTable.truncate(SystemSymbols.ION_1_0_MAX_ID + 1)
+        // TODO: Make this more efficient with an array copy operation.
+        additionalActiveModules.forEach { m -> m.symbols.forEach { newEffectiveSymbolTable.add(it) } }
+
+        spareTables.symbols = effectiveTables.symbols
+        effectiveTables.symbols = newEffectiveSymbolTable
+    }
+
+    private fun rebuildEffectiveMacroTable() {
+        TODO()
+    }
 }

src/main/java/com/amazon/ion/bytecode/bin10/ByteArrayBytecodeGenerator10.kt

@@ -31,6 +31,9 @@ import kotlin.math.min
 /**
  * A bytecode generator for Ion 1.0 binary encoding.
  *
+ * TODO: Consider _not_ stopping the refill when an Ion 1.0 IVM or Ion 1.0 symbol table is encountered, since it does
+ *       not affect the correctness. Only things that change the macro table require the refill to stop.
+ *
  * #### Note on integer values
  *
  * Because there are separate positive/negative opcodes for integers in Ion 1.0, the data referenced in an INT_REF

src/main/java/com/amazon/ion/bytecode/ir/Instructions.kt

@@ -48,6 +48,22 @@ internal object Instructions {
     @JvmStatic
     fun getData(instruction: Int) = instruction and DATA_MASK
 
+    /**
+     * Determines the number of operands based on the operand count bits and the `length` in the data bits.
+     *
+     * @param instruction The packed instruction integer
+     * @return The number of operands for this instruction.
+     */
+    @JvmStatic
+    fun getOperandCount(instruction: Int): Int {
+        val maybeLength = getData(instruction)
+        val operandCountBits = getOperandCountBits(instruction)
+        // equivalent to `i += if (operandsToSkip == 3) length else operandsToSkip`
+        // `useOperandCount` is all zeros if `operandsToSkip` is 3, and all ones if `operandsToCount` is smaller than 3.
+        val useOperandCount = ((operandCountBits - 3) shr 2)
+        return (operandCountBits and useOperandCount) or (maybeLength and useOperandCount.inv())
+    }
+
     /**
      * Packs a data value with an instruction to create a packed instruction.
      */