This is an automated email from the ASF dual-hosted git repository.
slawrence pushed a commit to branch main
in repository https://gitbox.apache.org/repos/asf/daffodil.git
The following commit(s) were added to refs/heads/main by this push:
new f3fb555c9 Reduce copies in VariableMap implementation
f3fb555c9 is described below
commit f3fb555c98aa3cfe24270f38b579b03316cc8ad0
Author: Steve Lawrence <[email protected]>
AuthorDate: Thu Oct 19 11:25:21 2023 -0400
Reduce copies in VariableMap implementation
The VariableMap currently uses a Scala Map to look up variable
instances, where the key is the QName of the variable and the value is
the variable instance stack.
One consideration with the data structure to be used is that it must be
frequently copied each time a suspension is created. So although fast
lookups are important, so are fast copies. And unfortunately, Map have
relatively expensive copies. A large backing array must be created,
buckets allocated, keys must be rehashed, etc. This overhead is enough
that it can be a significant contribution when profiling, especially in
formats with lots of suspensions.
To fix this, this replaces the Map with an Array, and each
VariableRuntimeData is assigned an index into this array. Now, instead
of looking up a variable by its QName, we just get its index from the
VRD and access that index in the array. This has the same constant time
lookup, but array copies are faster and require fewer and smaller
allocations.
Similarly, during suspensions we must also create an immutable copy of
each variable instance inside the array so that suspensions do not see
new variables instances. Variable instances are currently represented as
an ArrayBuffer, which has noticeable overhead when copying. Since this
ArrayBuffer is just used as a stack, this replaces it with a Seq. This
gives us similar stack-like behavior, but is immutable so copies are
free. This does mean however that new Seq's must be allocated if we
create a lot of newVariableInstances, but that is rare enough, and
still fairly fast, that the benefit of preallocating an ArrayBuffer
stack does not have significant benefits.
Note that the Seq of VariableRuntimeData is added as a member to the
VariableMap since in some cases (e.g. debugging, setting external
variables), we do need a way to lookup the VRD by QName. This will be
relatively slow, so any performance critical sections should find the
VRD during compilation and use that during runtime.
This also removes the VariableMapFactory, it doesn't add much value
except a level of indirection.
Also removes the allExternalVariables val. Nothing uses it and
VariableMap handles checking external variables itself by inspecting the
VariableRuntimeData. We don't need this and it is trivial enough to
implement if something else needs it.
In a large schema with small files and lots of suspensions, this saw
around a 50% improvement in unparse performance.
DAFFODIL-2852
---
.../org/apache/daffodil/core/dsom/SchemaSet.scala | 9 +-
.../core/runtime1/SchemaSetRuntime1Mixin.scala | 8 +-
.../core/runtime1/VariableMapFactory.scala | 30 --
.../core/runtime1/VariableRuntime1Mixin.scala | 2 +
.../org/apache/daffodil/core/util/TestUtils.scala | 3 +-
.../daffodil/runtime1/processors/RuntimeData.scala | 1 +
.../runtime1/processors/VariableMap1.scala | 313 ++++++++++-----------
.../daffodil/section07/variables/variables.tdml | 25 ++
.../section07/variables/TestVariables.scala | 2 +
9 files changed, 183 insertions(+), 210 deletions(-)
diff --git
a/daffodil-core/src/main/scala/org/apache/daffodil/core/dsom/SchemaSet.scala
b/daffodil-core/src/main/scala/org/apache/daffodil/core/dsom/SchemaSet.scala
index 7512dab21..6a76ae2f6 100644
--- a/daffodil-core/src/main/scala/org/apache/daffodil/core/dsom/SchemaSet.scala
+++ b/daffodil-core/src/main/scala/org/apache/daffodil/core/dsom/SchemaSet.scala
@@ -571,12 +571,9 @@ final class SchemaSet private (
Seq(encDFV, boDFV, binDFV, outDFV)
}
- lazy val allDefinedVariables = schemas.flatMap {
- _.defineVariables
- }
- lazy val allExternalVariables = allDefinedVariables.filter {
- _.external
- }
+ lazy val allDefinedVariables = schemas
+ .flatMap(_.defineVariables)
+ .union(predefinedVars)
private lazy val checkUnusedProperties = LV('hasUnusedProperties) {
root.checkUnusedProperties
diff --git
a/daffodil-core/src/main/scala/org/apache/daffodil/core/runtime1/SchemaSetRuntime1Mixin.scala
b/daffodil-core/src/main/scala/org/apache/daffodil/core/runtime1/SchemaSetRuntime1Mixin.scala
index 32eb6461d..7e26c0a27 100644
---
a/daffodil-core/src/main/scala/org/apache/daffodil/core/runtime1/SchemaSetRuntime1Mixin.scala
+++
b/daffodil-core/src/main/scala/org/apache/daffodil/core/runtime1/SchemaSetRuntime1Mixin.scala
@@ -18,7 +18,6 @@
package org.apache.daffodil.core.runtime1
import org.apache.daffodil.core.dsom.SchemaSet
-import org.apache.daffodil.core.grammar.VariableMapFactory
import org.apache.daffodil.lib.util.Logger
import org.apache.daffodil.runtime1.api.DFDL
import org.apache.daffodil.runtime1.processors.DataProcessor
@@ -36,11 +35,8 @@ trait SchemaSetRuntime1Mixin {
requiredEvaluationsAlways(root.elementRuntimeData.initialize)
override lazy val variableMap: VariableMap = LV('variableMap) {
- val dvs = allSchemaDocuments.flatMap {
- _.defineVariables
- }
- val alldvs = dvs.union(predefinedVars)
- val vmap = VariableMapFactory.create(alldvs)
+ val vrds = allDefinedVariables.map { _.variableRuntimeData }
+ val vmap = VariableMap(vrds)
vmap
}.value
diff --git
a/daffodil-core/src/main/scala/org/apache/daffodil/core/runtime1/VariableMapFactory.scala
b/daffodil-core/src/main/scala/org/apache/daffodil/core/runtime1/VariableMapFactory.scala
deleted file mode 100644
index 7ac3cfa42..000000000
---
a/daffodil-core/src/main/scala/org/apache/daffodil/core/runtime1/VariableMapFactory.scala
+++ /dev/null
@@ -1,30 +0,0 @@
-/*
- * Licensed to the Apache Software Foundation (ASF) under one or more
- * contributor license agreements. See the NOTICE file distributed with
- * this work for additional information regarding copyright ownership.
- * The ASF licenses this file to You under the Apache License, Version 2.0
- * (the "License"); you may not use this file except in compliance with
- * the License. You may obtain a copy of the License at
- *
- * http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing, software
- * distributed under the License is distributed on an "AS IS" BASIS,
- * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
- * See the License for the specific language governing permissions and
- * limitations under the License.
- */
-
-package org.apache.daffodil.core.grammar
-
-import org.apache.daffodil.core.dsom.DFDLDefineVariable
-import org.apache.daffodil.runtime1.processors.VariableMap
-
-object VariableMapFactory {
-
- def create(dvs: Seq[DFDLDefineVariable]): VariableMap = {
- val vrds = dvs.map { _.variableRuntimeData }
- val vmap = new VariableMap(vrds)
- vmap
- }
-}
diff --git
a/daffodil-core/src/main/scala/org/apache/daffodil/core/runtime1/VariableRuntime1Mixin.scala
b/daffodil-core/src/main/scala/org/apache/daffodil/core/runtime1/VariableRuntime1Mixin.scala
index 80ad9c6e1..6b0250f36 100644
---
a/daffodil-core/src/main/scala/org/apache/daffodil/core/runtime1/VariableRuntime1Mixin.scala
+++
b/daffodil-core/src/main/scala/org/apache/daffodil/core/runtime1/VariableRuntime1Mixin.scala
@@ -46,6 +46,7 @@ trait DFDLDefineVariableRuntime1Mixin { self:
DFDLDefineVariable =>
this.namedQName.asInstanceOf[GlobalQName],
this.primType,
this.tunable.unqualifiedPathStepPolicy,
+ this.schemaSet.allDefinedVariables.indexOf(this),
)
vrd
}
@@ -97,6 +98,7 @@ trait DFDLNewVariableInstanceRuntime1Mixin { self:
DFDLNewVariableInstance =>
defv.namedQName.asInstanceOf[GlobalQName],
defv.primType,
this.tunable.unqualifiedPathStepPolicy,
+ this.schemaSet.allDefinedVariables.indexOf(defv),
)
vrd
}
diff --git
a/daffodil-core/src/test/scala/org/apache/daffodil/core/util/TestUtils.scala
b/daffodil-core/src/test/scala/org/apache/daffodil/core/util/TestUtils.scala
index 812a240a0..b009e10b9 100644
--- a/daffodil-core/src/test/scala/org/apache/daffodil/core/util/TestUtils.scala
+++ b/daffodil-core/src/test/scala/org/apache/daffodil/core/util/TestUtils.scala
@@ -30,7 +30,6 @@ import scala.xml._
import org.apache.daffodil.core.compiler.Compiler
import org.apache.daffodil.core.dsom._
-import org.apache.daffodil.core.grammar.VariableMapFactory
import org.apache.daffodil.io.InputSourceDataInputStream
import org.apache.daffodil.lib.Implicits._
import org.apache.daffodil.lib.api._
@@ -389,7 +388,7 @@ class Fakes private () {
override def isError: Boolean = false
override def tunables: DaffodilTunables = DaffodilTunables()
- override def variableMap: VariableMap = VariableMapFactory.create(Nil)
+ override def variableMap: VariableMap = VariableMap(Nil)
override def validationMode: ValidationMode.Type = ValidationMode.Full
override def withExternalVariables(extVars: Seq[Binding]):
DFDL.DataProcessor = this
diff --git
a/daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/processors/RuntimeData.scala
b/daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/processors/RuntimeData.scala
index b00caeb65..5424638ed 100644
---
a/daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/processors/RuntimeData.scala
+++
b/daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/processors/RuntimeData.scala
@@ -980,6 +980,7 @@ final class VariableRuntimeData(
val globalQName: GlobalQName,
val primType: NodeInfo.PrimType,
unqualifiedPathStepPolicyArg: UnqualifiedPathStepPolicy,
+ val vmapIndex: Int,
) extends NonTermRuntimeData(
null, // no variable map
schemaFileLocationArg,
diff --git
a/daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/processors/VariableMap1.scala
b/daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/processors/VariableMap1.scala
index f00949393..4ae4cbbac 100644
---
a/daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/processors/VariableMap1.scala
+++
b/daffodil-runtime1/src/main/scala/org/apache/daffodil/runtime1/processors/VariableMap1.scala
@@ -17,9 +17,6 @@
package org.apache.daffodil.runtime1.processors
-import scala.collection.mutable.ArrayBuffer
-import scala.collection.mutable.Map
-
import org.apache.daffodil.lib.api.ThinDiagnostic
import org.apache.daffodil.lib.api.WarnID
import org.apache.daffodil.lib.exceptions.Assert
@@ -218,7 +215,17 @@ final class VariableBox(initialVMap: VariableMap) {
vmap_ = newMap
}
- def cloneForSuspension(): VariableBox = new
VariableBox(vmap.topLevelInstances)
+ def cloneForSuspension(): VariableBox = new
VariableBox(vmap.cloneForSuspension)
+}
+
+object VariableMap {
+ def apply(vrds: Seq[VariableRuntimeData] = Nil) = {
+ val table = new Array[Seq[VariableInstance]](vrds.size)
+ vrds.foreach { vrd =>
+ table(vrd.vmapIndex) = Seq(vrd.createVariableInstance())
+ }
+ new VariableMap(vrds, table)
+ }
}
/**
@@ -233,21 +240,26 @@ final class VariableBox(initialVMap: VariableMap) {
* The DPath implementation must be made to implement the
* no-set-after-default-value-has-been-read behavior. This requires that
reading the variables causes a state transition.
*
- * The implementation of the VariabeMap uses ArrayBuffers essentially as a
- * stack, as they allow for easy serialization unlike the custom MStack classes
- * we use elsewhere. Scala's mutable Stack is deprecated in 2.12
+ * The VariableMap implementation essentially uses Seq as a stack, as they
allow for easy
+ * serialization unlike the custom MStack classes we use elsewhere. Note that
Scala's mutable
+ * Stack is deprecated in 2.12. Additionally, this stack is rarely changed
(only when
+ * newVariableInstance occurs) so limiting allocations is not critical.
Further, suspensions
+ * create a copy of the vTable, which is a relatively expensive operation if
using something
+ * else like an ArrayBuffer or MStack, since Seq's are immutable and copies
are virtually free.
+ *
+ * Note that each index in the vTable array contains the VariableInstance
stack for a specific
+ * variable, with that index defined by the "vmapIndex" member of the variables
+ * VariableRuntimeData. This implementation allows for constant lookups by
indexing into the
+ * vTable array and accessing the head of the VariableInstance sequence.
Additionally, in some
+ * cases (like with suspensions) we have to make a copy of the array. This is
the primary reason
+ * for using an Array as opposed to another data structure that allows
constant lookups like a
+ * Map--array copies are quite fast, and we know the exact size since all
variables must be
+ * defined at compile time. Although this adds extra complexity compared to a
Map since we must
+ * carry around an index, the performance gains are worth it.
*/
-class VariableMap private (vTable: Map[GlobalQName,
ArrayBuffer[VariableInstance]])
+class VariableMap private (vrds: Seq[VariableRuntimeData], vTable:
Array[Seq[VariableInstance]])
extends Serializable {
- def this(topLevelVRDs: Seq[VariableRuntimeData] = Nil) =
- this(Map(topLevelVRDs.map { vrd =>
- val variab = vrd.createVariableInstance()
- val variableInstances = new ArrayBuffer[VariableInstance]
- variableInstances += variab
- (vrd.globalQName, variableInstances)
- }: _*))
-
override def toString(): String = {
"VariableMap(" + vTable.mkString(" | ") + ")"
}
@@ -257,26 +269,18 @@ class VariableMap private (vTable: Map[GlobalQName,
ArrayBuffer[VariableInstance
* VariableInstances are mutable and cannot safely be shared across threads
*/
def copy(): VariableMap = {
- val table = vTable.map {
- case (k: GlobalQName, variableInstances: ArrayBuffer[VariableInstance])
=> {
- val newBuf = variableInstances.map { _.copy() }
- (k, newBuf)
- }
+ val table = vTable.map { variableInstances =>
+ val newBuf = variableInstances.map { _.copy() }
+ newBuf
}
- new VariableMap(table)
+ new VariableMap(vrds, table)
}
- def topLevelInstances(): VariableMap = {
- val table = vTable.map {
- case (k: GlobalQName, variableInstances: ArrayBuffer[VariableInstance])
=> {
- val newBuf = new ArrayBuffer[VariableInstance]()
- newBuf.append(variableInstances.last)
- (k, newBuf)
- }
- }
-
- new VariableMap(table)
+ def cloneForSuspension(): VariableMap = {
+ val newTable = new Array[Seq[VariableInstance]](vTable.size)
+ Array.copy(vTable, 0, newTable, 0, vTable.size)
+ new VariableMap(vrds, newTable)
}
// For defineVariable's with non-constant expressions for default values, it
@@ -285,20 +289,11 @@ class VariableMap private (vTable: Map[GlobalQName,
ArrayBuffer[VariableInstance
// must also ensure that the expressions only reference other variables that
// have default value expressions or are defined externally.
def forceExpressionEvaluations(state: ParseOrUnparseState): Unit = {
- vTable.foreach {
- case (_, variableInstances) => {
- variableInstances.foreach { inst =>
- {
- (inst.state, inst.rd.maybeDefaultValueExpr.isDefined) match {
- // Evaluate defineVariable statements with non-constant default
value expressions
- case (VariableUndefined, true) => {
- val res = inst.rd.maybeDefaultValueExpr.get.evaluate(state)
- inst.setDefaultValue(DataValue.unsafeFromAnyRef(res))
- }
- case (_, _) => // Do nothing
- }
- }
- }
+ vTable.foreach { variableInstances =>
+ val inst = variableInstances.head
+ if ((inst.state eq VariableUndefined) &&
inst.rd.maybeDefaultValueExpr.isDefined) {
+ val res = inst.rd.maybeDefaultValueExpr.get.evaluate(state)
+ inst.setDefaultValue(DataValue.unsafeFromAnyRef(res))
}
}
}
@@ -311,31 +306,33 @@ class VariableMap private (vTable: Map[GlobalQName,
ArrayBuffer[VariableInstance
* expression
*/
def setFirstInstanceInitialValues(): Unit = {
- vTable.foreach {
- case (_, variableInstances) => {
- variableInstances(0).firstInstanceInitialValue =
variableInstances(0).value
- }
+ vTable.foreach { variableInstances =>
+ variableInstances.head.firstInstanceInitialValue =
variableInstances.head.value
}
}
+ /**
+ * Performance of this is linear in number of variables, this should not be
used in
+ * performance critical sections.
+ */
def find(qName: GlobalQName): Option[VariableInstance] = {
- val optBuf = vTable.get(qName)
- val variab = {
- if (optBuf.isDefined)
- Some(optBuf.get.last)
- else
- None
- }
- variab
+ getVariableRuntimeData(qName).map { vrd => vTable(vrd.vmapIndex).head }
}
- def qnames(): Seq[GlobalQName] = {
- vTable.toSeq.map(_._1)
+ /**
+ * Performance of this is linear in number of variables, this should not be
used in
+ * performance critical sections.
+ */
+ lazy val qnames: Seq[GlobalQName] = {
+ vrds.map { _.globalQName }
}
+ /**
+ * Performance of this is linear in number of variables, this should not be
used in
+ * performance critical sections.
+ */
def getVariableRuntimeData(qName: GlobalQName): Option[VariableRuntimeData]
= {
- val optVariable = find(qName)
- if (optVariable.isDefined) Some(optVariable.get.rd) else None
+ vrds.find { _.globalQName == qName }
}
lazy val context = Assert.invariantFailed("unused.")
@@ -348,14 +345,9 @@ class VariableMap private (vTable: Map[GlobalQName,
ArrayBuffer[VariableInstance
* perform
*/
def readVariableWillChangeState(vrd: VariableRuntimeData): Boolean = {
- val variableInstances = vTable.get(vrd.globalQName)
- if (variableInstances.isDefined) {
- val variable = variableInstances.get.last
- val variableRead = (variable.state eq VariableRead) &&
variable.value.isDefined
- !variableRead
- } else {
- true
- }
+ val variable = vTable(vrd.vmapIndex).head
+ val variableRead = (variable.state eq VariableRead) &&
variable.value.isDefined
+ !variableRead
}
/**
@@ -384,41 +376,34 @@ class VariableMap private (vTable: Map[GlobalQName,
ArrayBuffer[VariableInstance
case _ => // Do nothing
}
- val variableInstances = vTable.get(varQName)
- if (variableInstances.isDefined) {
- val variable = variableInstances.get.last
- variable.state match {
- case VariableRead if (variable.value.isDefined) =>
variable.value.getNonNullable
- case VariableDefined | VariableSet if (variable.value.isDefined) => {
- variable.setState(VariableRead)
- variable.value.getNonNullable
- }
- // This case is only hit for defineVariable's who's expression
reference
- // other defineVariables with expressions. It will be hit at the start
- // of parsing, before an infoset is generated, via the
- // forceExpressionEvaluations function after which all variables should
- // have a defined value
- case VariableUndefined if
(variable.rd.maybeDefaultValueExpr.isDefined) => {
- variable.setState(VariableBeingDefined)
- val res =
-
DataValue.unsafeFromAnyRef(variable.rd.maybeDefaultValueExpr.get.evaluate(state))
-
- // Need to update the variable's value with the result of the
- // expression
- variable.setState(VariableRead)
- variable.setValue(res)
-
- res
- }
- case VariableBeingDefined => throw new
VariableCircularDefinition(varQName, vrd)
- case VariableInProcess => throw new VariableSuspended(varQName, vrd)
- case _ => throw new VariableHasNoValue(varQName, vrd)
+ val variable = vTable(vrd.vmapIndex).head
+ variable.state match {
+ case VariableRead if (variable.value.isDefined) =>
variable.value.getNonNullable
+ case VariableDefined | VariableSet if (variable.value.isDefined) => {
+ variable.setState(VariableRead)
+ variable.value.getNonNullable
+ }
+ // This case is only hit for defineVariable's who's expression reference
+ // other defineVariables with expressions. It will be hit at the start
+ // of parsing, before an infoset is generated, via the
+ // forceExpressionEvaluations function after which all variables should
+ // have a defined value
+ case VariableUndefined if (variable.rd.maybeDefaultValueExpr.isDefined)
=> {
+ variable.setState(VariableBeingDefined)
+ val res =
+
DataValue.unsafeFromAnyRef(variable.rd.maybeDefaultValueExpr.get.evaluate(state))
+
+ // Need to update the variable's value with the result of the
+ // expression
+ variable.setState(VariableRead)
+ variable.setValue(res)
+
+ res
}
- } else
- referringContext.SDE(
- "Variable map (compilation): unknown variable %s",
- varQName,
- ) // Fix DFDL-766
+ case VariableBeingDefined => throw new
VariableCircularDefinition(varQName, vrd)
+ case VariableInProcess => throw new VariableSuspended(varQName, vrd)
+ case _ => throw new VariableHasNoValue(varQName, vrd)
+ }
}
/**
@@ -431,59 +416,57 @@ class VariableMap private (vTable: Map[GlobalQName,
ArrayBuffer[VariableInstance
pstate: ParseOrUnparseState,
) = {
val varQName = vrd.globalQName
- val variableInstances = vTable.get(varQName)
- if (variableInstances.isDefined) {
- val variable = variableInstances.get.last
- variable.state match {
- case VariableSet => {
- referringContext.SDE(
- "Cannot set variable %s twice. State was: %s. Existing value: %s",
- variable.rd.globalQName,
- VariableSet,
- variable.value,
- )
- }
+ val variableInstances = vTable(vrd.vmapIndex)
+ val variable = variableInstances.head
+ variable.state match {
+ case VariableSet => {
+ referringContext.SDE(
+ "Cannot set variable %s twice. State was: %s. Existing value: %s",
+ variable.rd.globalQName,
+ VariableSet,
+ variable.value,
+ )
+ }
- case VariableRead => {
+ case VariableRead => {
+
+ /**
+ * TODO: This should be an SDE, but due to a bug (DAFFODIL-1443) in
+ * the way we evaluate escapeSchemes it could lead us to setting the
+ * variable read too early */
+ pstate.SDW(
+ WarnID.VariableSet,
+ "Cannot set variable %s after reading the default value. State was:
%s. Existing value: %s",
+ variable.rd.globalQName,
+ VariableSet,
+ variable.value,
+ )
+ variable.setValue(newValue)
+ variable.setState(VariableSet)
+ }
+ case _ => {
+ vrd.direction match {
/**
- * TODO: This should be an SDE, but due to a bug (DAFFODIL-1443) in
- * the way we evaluate escapeSchemes it could lead us to setting the
- * variable read too early */
- pstate.SDW(
- WarnID.VariableSet,
- "Cannot set variable %s after reading the default value. State
was: %s. Existing value: %s",
- variable.rd.globalQName,
- VariableSet,
- variable.value,
- )
- variable.setValue(newValue)
- variable.setState(VariableSet)
- }
-
- case _ => {
- vrd.direction match {
- /**
- * Due to potential race conditions regarding the setting of
- * variables via setVariable and default values in combination with
- * suspensions during unparsing, we only allow the use of either
- * setVariable statements or a default value when unparsing a
- * variable.
- */
- case VariableDirection.UnparseOnly | VariableDirection.Both
- if (vrd.maybeDefaultValueExpr.isDefined &&
variableInstances.get.size > 1) => {
- // Variable has an unparse direction, a default value, and a
- // newVariableInstance
- pstate.SDE(
- "Variable %s has an unparse direction and a default value,
setting the variable may cause race conditions when combined with a forward
referencing expression.",
- varQName,
- )
- }
- case _ => // Do nothing
+ * Due to potential race conditions regarding the setting of
+ * variables via setVariable and default values in combination with
+ * suspensions during unparsing, we only allow the use of either
+ * setVariable statements or a default value when unparsing a
+ * variable.
+ */
+ case VariableDirection.UnparseOnly | VariableDirection.Both
+ if (vrd.maybeDefaultValueExpr.isDefined &&
variableInstances.size > 1) => {
+ // Variable has an unparse direction, a default value, and a
+ // newVariableInstance
+ pstate.SDE(
+ "Variable %s has an unparse direction and a default value,
setting the variable may cause race conditions when combined with a forward
referencing expression.",
+ varQName,
+ )
}
- variable.setValue(newValue)
- variable.setState(VariableSet)
+ case _ => // Do nothing
}
+ variable.setValue(newValue)
+ variable.setState(VariableSet)
}
}
}
@@ -492,20 +475,17 @@ class VariableMap private (vTable: Map[GlobalQName,
ArrayBuffer[VariableInstance
* Creates a new instance of a variable without default value
*/
def newVariableInstance(vrd: VariableRuntimeData): VariableInstance = {
- val varQName = vrd.globalQName
- val variableInstances = vTable.get(varQName)
- Assert.invariant(variableInstances.isDefined)
+ val variableInstances = vTable(vrd.vmapIndex)
val nvi = vrd.createVariableInstance()
- nvi.firstInstanceInitialValue =
variableInstances.get(0).firstInstanceInitialValue
- variableInstances.get += nvi
+ nvi.firstInstanceInitialValue =
variableInstances.head.firstInstanceInitialValue
+ vTable(vrd.vmapIndex) = nvi +: variableInstances
nvi
}
def removeVariableInstance(vrd: VariableRuntimeData): Unit = {
- val varQName = vrd.globalQName
- val variableInstances = vTable.get(varQName)
- Assert.invariant(variableInstances.isDefined &&
variableInstances.get.nonEmpty)
- variableInstances.get.trimEnd(1)
+ val variableInstances = vTable(vrd.vmapIndex)
+ Assert.invariant(variableInstances.nonEmpty)
+ vTable(vrd.vmapIndex) = variableInstances.tail
}
/**
@@ -524,21 +504,22 @@ class VariableMap private (vTable: Map[GlobalQName,
ArrayBuffer[VariableInstance
// solving this ambiguity if only one of those varibles was external,
// but it's safer to err on the side of caution and require the user to
// be explicit about which variable they intended to set.
- val candidates = vTable.filter { case (key, _) =>
- key.local == bindingQName.local
+ val candidates = vrds.filter { vrd =>
+ vrd.globalQName.local == bindingQName.local
}
candidates.size match {
case 0 => None
- case 1 => Some(candidates.head._2)
+ case 1 => Some(vTable(candidates.head.vmapIndex))
case _ => {
val msg =
"External variable binding %s is ambiguous. A namespace is
required to resolve the ambiguity. Found variables: %s"
- .format(bindingQName,
candidates.keys.map(_.toString).mkString(", "))
+ .format(bindingQName,
candidates.map(_.globalQName.toString).mkString(", "))
throw new ExternalVariableException(msg)
}
}
} else {
- vTable.get(bindingQName.toGlobalQName)
+ val optVrd = vrds.find { _.globalQName == bindingQName.toGlobalQName }
+ optVrd.map { vrd => vTable(vrd.vmapIndex) }
}
optVariableInstances match {
diff --git
a/daffodil-test/src/test/resources/org/apache/daffodil/section07/variables/variables.tdml
b/daffodil-test/src/test/resources/org/apache/daffodil/section07/variables/variables.tdml
index 88b784f51..290b5540a 100644
---
a/daffodil-test/src/test/resources/org/apache/daffodil/section07/variables/variables.tdml
+++
b/daffodil-test/src/test/resources/org/apache/daffodil/section07/variables/variables.tdml
@@ -2754,5 +2754,30 @@
</tdml:infoset>
</tdml:parserTestCase>
+ <tdml:defineSchema name="backwardsDefines">
+ <!--
+ Daffodil evaluates variable defaults in the order the variables are
+ defined. This schema ensures that variable defaults that reference later
+ defined variables still works, i.e. defineVariable order does not matter
+ -->
+ <dfdl:defineVariable name="three" type="xs:int" defaultValue="{ $two + 1
}" />
+ <dfdl:defineVariable name="two" type="xs:int" defaultValue="{ $one + 1 }"
/>
+ <dfdl:defineVariable name="one" type="xs:int" defaultValue="1" />
+
+ <xs:include
schemaLocation="/org/apache/daffodil/xsd/DFDLGeneralFormat.dfdl.xsd"/>
+ <dfdl:format ref="ex:GeneralFormat" />
+
+ <xs:element name="three" type="xs:int" dfdl:inputValueCalc="{ $three }" />
+
+ </tdml:defineSchema>
+
+ <tdml:parserTestCase name="backwardsDefines_01" root="three"
model="backwardsDefines">
+ <tdml:document></tdml:document>
+ <tdml:infoset>
+ <tdml:dfdlInfoset>
+ <three>3</three>
+ </tdml:dfdlInfoset>
+ </tdml:infoset>
+ </tdml:parserTestCase>
</tdml:testSuite>
diff --git
a/daffodil-test/src/test/scala/org/apache/daffodil/section07/variables/TestVariables.scala
b/daffodil-test/src/test/scala/org/apache/daffodil/section07/variables/TestVariables.scala
index d87c0ebfc..166e0aa48 100644
---
a/daffodil-test/src/test/scala/org/apache/daffodil/section07/variables/TestVariables.scala
+++
b/daffodil-test/src/test/scala/org/apache/daffodil/section07/variables/TestVariables.scala
@@ -165,6 +165,8 @@ class TestVariables {
@Test def test_nviInGroupDecl_01(): Unit = {
runner.runOneTest("nviInGroupDecl_01") }
+ @Test def test_backwardsDefines_01: Unit = {
runner.runOneTest("backwardsDefines_01") }
+
/*****************************************************************/
val tdmlVal = XMLUtils.TDML_NAMESPACE
val dfdl = XMLUtils.DFDL_NAMESPACE
