stevedlawrence commented on a change in pull request #437:
URL: https://github.com/apache/incubator-daffodil/pull/437#discussion_r505641804
##########
File path:
daffodil-runtime1-unparser/src/main/scala/org/apache/daffodil/processors/unparsers/ElementUnparser.scala
##########
@@ -417,39 +418,56 @@ sealed trait RegularElementUnparserStartEndStrategy
UnparseError(Nope, One(state.currentLocation), "Expected element
start event for %s, but received %s.",
erd.namedQName.toExtendedSyntax, event)
}
- val res = event.info.element
- val mCurNode = state.currentInfosetNodeMaybe
- if (mCurNode.isDefined) {
- val c = mCurNode.get.asComplex
- Assert.invariant(!c.isFinal)
- if (c.maybeIsNilled == MaybeBoolean.True) {
- // cannot add content to a nilled complex element
- UnparseError(One(erd.schemaFileLocation), Nope, "Nilled complex
element %s has content from %s",
- c.erd.namedQName.toExtendedSyntax,
- res.erd.namedQName.toExtendedSyntax)
- }
- c.addChild(res, state.tunable)
- } else {
- val doc = state.documentElement
- doc.addChild(res, state.tunable) // DIDocument, which is never a
current node, must have the child added
- doc.isFinal = true // that's the only child.
- }
- res
+ event.info.element
} else {
Assert.invariant(state.withinHiddenNest)
// Since we never get events for elements in hidden contexts, their
infoset elements
// will have never been created. This means we need to manually
create them
- val e = if (erd.isComplexType) new DIComplex(erd) else new
DISimple(erd)
- e.setHidden()
- state.currentInfosetNode.asComplex.addChild(e, state.tunable)
- e
+ val hiddenElem = if (erd.isComplexType) new DIComplex(erd) else new
DISimple(erd)
+ hiddenElem.setHidden()
+ hiddenElem
+ }
+
+ // now add this new elem to the infoset
+ val parentNodeMaybe = state.currentInfosetNodeMaybe
+ if (parentNodeMaybe.isDefined) {
+ val parentComplex = parentNodeMaybe.get.asComplex
+ Assert.invariant(!parentComplex.isFinal)
+ if (parentComplex.maybeIsNilled == MaybeBoolean.True) {
+ // cannot add content to a nilled complex element
+ UnparseError(One(erd.schemaFileLocation), Nope, "Nilled complex
element %s has content from %s",
+ parentComplex.erd.namedQName.toExtendedSyntax,
+ newElem.erd.namedQName.toExtendedSyntax)
}
+ // We are about to add a child to this complex element. Before we do
+ // that, if the last child added to this complex is a DIArray, and this
+ // new child isn't part of that array, that implies that the DIArray
+ // will have no more children added and should be marked as final, and
Review comment:
> So there's some confusion here around the role of isFinal in my mind.
They do feel separate, but are actually closely related. isFinal is only set
for elements when unparseEnd is called, which means the element must have been
fully unparsed and can be removed. The place where this sort feels question is
with suspensions. The unparse process looks like this:
1. parseBegin - create an Element and add to infoset
2. Unparse body, potentially creating a Suspension
3. parseEnd - mark element as isFInal and free
So I think a concern is that we might unparse the body and create a
suspension, but then immediately call parseEnd and free the element. However,
creating a suspension clones the current infoset, so while the element is freed
from the "main" infoset, the suspension still has a pointer to it and can do
the normal processing.
> If we wait for an entire array to be isFinal before we free any of its
child elements, then we cannot truly stream a large array. Freeing element with
index N-1 upon unparse of element N seems essential to obtain streaming
behavior.
We do not wait for an array to be final before freeing any of it's children.
We free the children as soon as they are done unparsing, because those children
will call unparseEnd, get marked as final, and they will be freed. Only when
all those children are finished and we add something after the array is the
arrary marked as isFInal and the array can be freed.
> I would expect we would pass the array index down to the
freeChildIfNoLongerNeeded call, since without that it can't decide to release
things based on index.
We do not currently free anything based on index. We free things as soon as
possible, including children of arrays.
The one case were don't free things (in both parse and unparse) is when
something is used in an expression. So large arrays are only a problem if they
are use in expressions. And even then, we still only keep the things in those
arrays that are used in expressions. So it's still wasteful, and large arrays
can cause problems, but it's certainly less of an issue.
There is a proposal to add a new dfdlx property that gives hints to Daffodil
to say that if an array with this property is used in an expression (and thus
we can't free it's contents) then we only need to keep the most recent X
elements. This isn't implemented yet. Perhaps this can be postponed for a
future release?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
