[GitHub] [daffodil] stevedlawrence commented on a change in pull request #682: Improve how we capture and restore variable maps in points of uncertainty

[GitBox <gi...@apache.org>]

stevedlawrence commented on a change in pull request #682:
URL: https://github.com/apache/daffodil/pull/682#discussion_r752593023



##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/parsers/PState.scala
##########
@@ -328,24 +308,51 @@ final class PState private (
     this.infoset = newParent
   }
 
+  /**
+   * When we take marks of this PState when we enter a PoU, we intentionally do
+   * not take a deep copy of the variable map because that is too expensive of
+   * an operation for a data structure that rarely changes. So taking a mark
+   * just makes a shallow copy of the PState variable map.
+   *
+   * But this means modifications of the variable map could potentially affect
+   * PoU marks, which means resetting back to a mark does not work as intended.
+   * To resolve this issue, anytime a function is called that will change the
+   * state of a variable, we must first call this function. This will take a
+   * deep copy of the variable map and set that copy as the variable map for
+   * this PState. This way, the state already captured in the mark will not be
+   * modified and we can correctly reset back to that state when resetting the
+   * PoU. Note that we only do this if we have not already done a deep copy in
+   * this PoU--if we've already done a deep copy, we don't need to do it again
+   * since the PoU copy can't be modified.
+   */
+  @inline
+  private def changingVariable(): Unit = {
+    if (!pointsOfUncertainty.isEmpty) {
+      val curPoU = pointsOfUncertainty.top
+      if (curPoU.variableMap eq this.variableMap) {
+        this.setVariableMap(this.variableMap.copy())
+      }
+    }
+  }
+
   override def setVariable(vrd: VariableRuntimeData, newValue: DataValuePrimitive, referringContext: ThrowsSDE): Unit = {
+    changingVariable()
     variableMap.setVariable(vrd, newValue, referringContext, this)
-    changedVariablesStack.top += vrd.globalQName
   }
 
   override def getVariable(vrd: VariableRuntimeData, referringContext: ThrowsSDE): DataValuePrimitive = {
-    val res = variableMap.readVariable(vrd, referringContext, this)
-    changedVariablesStack.top += vrd.globalQName
-    res
+    changingVariable()
+    variableMap.readVariable(vrd, referringContext, this)
   }
 
   def newVariableInstance(vrd: VariableRuntimeData): VariableInstance = {
-    val v = variableMap.newVariableInstance(vrd)
-    changedVariablesStack.top += vrd.globalQName
-    v
+    changingVariable()

Review comment:
       newVariableInstance does modify the variable map, it's code looks like this:
   ```scala
       val varQName = vrd.globalQName
       val stack = vTable.get(varQName)
       Assert.invariant(stack.isDefined)
       stack.get.push(vrd.createVariableInstance)
   ```
   So it gets the stack of instances associated with the variable qname and then pushes a new instance. The new instances state is likely empty until it's set/read, but it has changed the variable map in a way that needs to be undone if we backtrack.




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: commits-unsubscribe@daffodil.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org