stevedlawrence commented on a change in pull request #682:
URL: https://github.com/apache/daffodil/pull/682#discussion_r752642500
##########
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:
After some testing, it doesn't seem to make a noticable difference. My
guess is the schema/data I used just doesn't read enough variables or have
enough PoUs for any differences to show up. But avoiding the deep copy seems
like and obvious optimization, and the extra check doesn't seem to make things
worse. I've added it to this change.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
