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

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

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



##########
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()

Review comment:
       Can this be optimized so that reading a variable that has already been read is not a changingVariable()? Or is that not necessary because the overhead is minimal to call this because it will detect it has already happened and just return?

##########
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:
       This seems odd to me. Creating a new variable instance doesn't change any existing variable. Once the new variable instance exists, then yes, that instance's value can be defaulted or set, and then read, but why does creating a new instance "change" anything?




-- 
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