You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@daffodil.apache.org by GitBox <gi...@apache.org> on 2020/09/14 13:56:53 UTC
[GitHub] [incubator-daffodil] stevedlawrence opened a new pull request #419: Reduce performance degradation caused by commit b0f59ef7c8
stevedlawrence opened a new pull request #419:
URL: https://github.com/apache/incubator-daffodil/pull/419
- Remove the DIFinalizable trait and setFinal function and replace with
direct modification of isFinal. Scala created some pretty inefficient
byte code for this which actually made a noticeable difference in
performance when removed.
- Add function to DINodes to efficiently return a Maybe of the last
child. This replaces the lastOption and avoids an Option allocation
- Remove the containerNode def and just keep track of the container node
in the InfosetWalker with a new stack. Avoids virtual function calls
and instanceOf/asInstanceOf.
- Skip batches of walk() calls. This avoids the overhead related to step
logic when no progress is being made due to PoU's and allows batching
of steps rather than stepping each time a infoset node is added.
- Replace the various infoset walker objects with functions. Rather than
returning objects in a Maybe and calling a function in those optionst,
just call the functions directly and return a boolean that says
whether a step was taken. Essentially the same logic, but avoids
object creation and virtual functions.
- Avoid accessing the top of the infoset walker stacks multiple times.
Instead, access once and pass down as parameters. These stack accesses
actually showed up quite a bit in profiling
- Remove match/case on types, and instead use functions to imply the
type. Avoids instanceOf/asInstanceOf which have a noticeable
overhead in inner loops
- Add a new setTop function to modify the top of an MStack. Avoids logic
to pop and then push a new value.
- Remove extra logic about saved last child node, this didn't make much
of a difference in when we called set final, and just added extra
overhead
- Remove calls to walk() made when PoU's were resolved. Instead, only
walk() when new elements are added. Elements are added all the time so
these extra calls just added overhead. The new step skipping logic
also helps with this.
DAFFODIL-2396
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-daffodil] stevedlawrence merged pull request #419: Reduce performance degradation caused by commit b0f59ef7c8
Posted by GitBox <gi...@apache.org>.
stevedlawrence merged pull request #419:
URL: https://github.com/apache/incubator-daffodil/pull/419
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-daffodil] stevedlawrence commented on a change in pull request #419: Reduce performance degradation caused by commit b0f59ef7c8
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #419:
URL: https://github.com/apache/incubator-daffodil/pull/419#discussion_r488072309
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/infoset/InfosetWalker.scala
##########
@@ -201,26 +229,60 @@ class InfosetWalker private (
* possible.
*
* It is an error to call walk() if isFinished returns true
+ *
+ * For performance reasons, sometimes a call to walk will intentionally not
+ * try to take any steps even if it is possible. This lets us essentially
+ * batch infoset events and improve performance. If this is the last time
+ * walk() will be called, the lastWalk parameter should be set to true, which
+ * will cause walk() to not skip any steps.
*/
- def walk(): Unit = {
+ def walk(lastWalk: Boolean = false): Unit = {
Assert.usage(!finished)
- var maybeNextStep: Maybe[InfosetWalkerStep] = Nope
- while ({
- maybeNextStep = getNextStep()
- maybeNextStep.isDefined
- }) {
- maybeNextStep.get.step()
+ if (walkSkipRemaining > 0 && !lastWalk) {
Review comment:
That's correct. The purpose of this is to avoid performing walks() when progress wont' be made.
This algorithm is essentially donig "call only after some large N elements are created" since we now only call walk after elements are created. It's a little smarter since it increases N if we fail to make progress, and then decreases once progress is made. Currently that N starts out at 32 and we double each time progress isn't made up to a max of 2048.
We could also check it there are PoU's, but maybeDoStep essentially does that, but also takes into account other kinds of blocks. This also allows us to increase how many calls to walk we skip when this does detect that there's somethign blocking from walking.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-daffodil] stevedlawrence commented on a change in pull request #419: Reduce performance degradation caused by commit b0f59ef7c8
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #419:
URL: https://github.com/apache/incubator-daffodil/pull/419#discussion_r488078655
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/parsers/SequenceParserBases.scala
##########
@@ -203,31 +201,27 @@ abstract class SequenceParserBase(
pstate.mpstate.moveOverOneGroupIndexOnly()
}
- val newLastChildNode = pstate.infoset.contents.lastOption
- if (newLastChildNode != savedLastChildNode) {
- // We have added at least one child to to this complex during
+ val newLastChildNode = pstate.infoset.maybeLastChild
+ if (newLastChildNode.isDefined) {
+ // We have potentially added a child to to this complex during
// this array loop.
//
// If the new child is a DIArray, we know this DIArray has at
// least one element, but we don't know if we actually added a
// new one in this loop or not. So just get the last array
- // element and set it as final anyways. Note that we need a null
- // check because if we didn't add an array element this go
- // around, the last element may have already been walked and
- // freed and so could now be null.
+ // element and set it as final anyways.
//
// If it's not a DIArray, that means it's just an optional
// simple/complex and that will get set final below where all
// other non-array elements get set as final.
- newLastChildNode.get match {
- case a: DIArray => {
- val lastArrayElem = a.contents.last
- if (lastArrayElem != null) {
- lastArrayElem.setFinal()
- pstate.walker.walk()
- }
+ val lastChild = newLastChildNode.get
+ if (!lastChild.isSimple && !lastChild.isComplex) {
Review comment:
Yeah, I'm surprised to. Of all the changes, I think this one didn't make a huge difference, it was definitely withing the margin of error for Java. But it did seem to consistent error on the positive side of that difference so I figured I'd keep it in to try to eek out every bit of performance. I'll add an isArray so this is at least more readable.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-daffodil] stevedlawrence commented on a change in pull request #419: Reduce performance degradation caused by commit b0f59ef7c8
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #419:
URL: https://github.com/apache/incubator-daffodil/pull/419#discussion_r488069432
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/infoset/InfosetWalker.scala
##########
@@ -178,7 +179,11 @@ class InfosetWalker private (
* we increment this value on the top of the stack so that the starting index
* is correct.
*/
- private var containerNode: DINode = startingContainerNode
+ private var containerNodeStack: MStackOf[DINode] = {
Review comment:
I think the issue was that containerNode requires a DINode, but parent is an InfosetElement, so we have to do instanceOf/asInstanceOf checks. Whatever bytecode this turns into appears to be relatively slow. If we got rid of all the InfosetElement distinction, we could probably get rid of alot of this.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-daffodil] mbeckerle commented on a change in pull request #419: Reduce performance degradation caused by commit b0f59ef7c8
Posted by GitBox <gi...@apache.org>.
mbeckerle commented on a change in pull request #419:
URL: https://github.com/apache/incubator-daffodil/pull/419#discussion_r488001832
##########
File path: daffodil-runtime1-unparser/src/main/scala/org/apache/daffodil/processors/unparsers/ElementUnparser.scala
##########
@@ -477,7 +477,7 @@ sealed trait RegularElementUnparserStartEndStrategy
}
val cur = state.currentInfosetNode
if (cur.isComplex)
- cur.asComplex.setFinal()
+ cur.isFinal = true
Review comment:
So the issue here is the finalizable mixin, resulting in virtual functions, when just a isFinal data member is not done by way of mixin?
So the guidance I guess is "fewer 'thin' mixins" in the runtime? You have to really need the mixin for it to be worth the overhead?
I''m trying to figure out what lessons learned, and what guidance we put in the coding standards notes, and try to remember to suggest during code reviews.
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/infoset/InfosetImpl.scala
##########
@@ -1458,6 +1454,12 @@ sealed class DIComplex(override val erd: ElementRuntimeData)
override def children = childNodes.toStream
+ override def maybeLastChild: Maybe[DINode] = {
+ val len = childNodes.length
+ if (len > 0) Maybe(childNodes(len - 1))
Review comment:
Add comment here that if the childNode has been removed (set to null), then Maybe(childNodes(len - 1 )) will be Maybe(null) which is Nope, and that this is expected behavior.
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/infoset/InfosetWalker.scala
##########
@@ -321,132 +390,114 @@ class InfosetWalker private (
}
}
- private trait InfosetWalkerStep {
- /**
- * Output events associated with this step kind, and mutate the
- * InfosetWalker state to walk to the next node in the infoset
- */
- def step(): Unit
-
- final def moveToFirstChild(newContainer: DINode): Unit = {
- containerNode = newContainer
- containerIndexStack.push(0)
- }
+ @inline
+ private def moveToFirstChild(newContainer: DINode): Unit = {
+ containerNodeStack.push(newContainer)
+ containerIndexStack.push(0)
+ }
- final def moveToContainer(): Unit = {
- containerNode = containerNode.containerNode
- containerIndexStack.pop
- }
+ @inline
+ private def moveToContainer(): Unit = {
+ containerNodeStack.pop
+ containerIndexStack.pop
+ }
- final def moveToNextSibling(): Unit = {
- containerIndexStack.push(containerIndexStack.pop + 1)
- }
+ @inline
+ private def moveToNextSibling(): Unit = {
+ val top = containerIndexStack.top
+ containerIndexStack.setTop(top + 1)
}
- private object InfosetWalkerStepStart extends InfosetWalkerStep {
- /**
- * Start the document. Note that because the top of container index is
- * initialized to one less that the starting index, we also call
- * moveToNextSibling to increment the starting index to the correct
- * position.
- */
- override def step(): Unit = {
- outputter.startDocument()
- moveToNextSibling()
- }
+ /**
+ * Start the document. Note that because the top of container index is
+ * initialized to one less that the starting index, we also call
+ * moveToNextSibling to increment the starting index to the correct
+ * position.
+ */
+ private def infosetWalkerStepStart(): Unit = {
+ outputter.startDocument()
+ moveToNextSibling()
}
- private object InfosetWalkerStepEnd extends InfosetWalkerStep {
- /**
- * End document and clean up state. Setting finished to true causes
- * the next step to be None, walk() will return, and the caller
- * should not call walk() again because it is finished.
- */
- override def step(): Unit = {
- outputter.endDocument()
- containerNode = null
- containerIndexStack = null
- finished = true
- }
+ /**
+ * End document and clean up state. Setting finished to true causes
+ * the next step to be None, walk() will return, and the caller
+ * should not call walk() again because it is finished.
+ */
+ private def infosetWalkerStepEnd(): Unit = {
+ outputter.endDocument()
+ containerNodeStack = null
+ containerIndexStack = null
+ finished = true
}
- private object InfosetWalkerStepMove extends InfosetWalkerStep {
- /**
- * Output start/end events for DIComplex/DIArray/DISimple, and mutate state
- * so we are looking at the next node in the infoset.
- */
- def step(): Unit = {
- val children = containerNode.contents
- val childIndex = containerIndexStack.top
-
- if (childIndex < children.size) {
- // This block means we need to create a start event for the element in
- // the children array at childIndex. We then need to mutate the walker
- // state so the next call to step() is for either the first child of
- // this element or the next sibling.
-
- children(childIndex) match {
- case s: DISimple => {
- if (!s.isHidden || walkHidden) {
- outputter.startSimple(s)
- outputter.endSimple(s)
- }
- if (removeUnneeded) {
- // now we can remove this simple element to free up memory
- containerNode.freeChildIfNoLongerNeeded(containerIndexStack.top)
- }
- moveToNextSibling()
- }
- case c: DIComplex => {
- if (!c.isHidden || walkHidden) {
- outputter.startComplex(c)
- moveToFirstChild(c)
- } else {
- moveToNextSibling()
- }
- }
- case a: DIArray => {
- if (!a.isHidden || walkHidden) {
- outputter.startArray(a)
- moveToFirstChild(a)
- } else {
- moveToNextSibling()
- }
- }
- }
+ /**
+ * Output start/end events for DIComplex/DIArray/DISimple, and mutate state
+ * so we are looking at the next node in the infoset.
+ */
+ private def infosetWalkerStepMove(containerNode: DINode, containerIndex: Int): Unit = {
+ val children = containerNode.contents
- } else {
- // This else block means that we incremented the index stack past the
- // number of children in this container (must be a DIComplex/DIDocument
- // or DIArray), which means we have created events for all if its
- // children. So we must now create the appropriate end event for the
- // container and then mutate the state so that we are looking at its
- // next sibling. Note that if there is no next sibling, that will be
- // found the next time step() is called (because we incremented past
- // this element) and we will fall into this block again, call the end
- // function again, and repeat the process.
-
- // create appropriate end event
- containerNode match {
- case a: DIArray => {
- outputter.endArray(a)
- }
- case c: DIComplex => {
- outputter.endComplex(c)
- }
- case _ => Assert.impossible()
- }
+ if (containerIndex < children.size) {
+ // This block means we need to create a start event for the element in
+ // the children array at containerIndex. We then need to mutate the walker
+ // state so the next call to step() is for either the first child of
+ // this element or the next sibling.
Review comment:
Don't understand "or the next sibling". What does that mean, or rather, why would that be the case?
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/processors/parsers/SequenceParserBases.scala
##########
@@ -203,31 +201,27 @@ abstract class SequenceParserBase(
pstate.mpstate.moveOverOneGroupIndexOnly()
}
- val newLastChildNode = pstate.infoset.contents.lastOption
- if (newLastChildNode != savedLastChildNode) {
- // We have added at least one child to to this complex during
+ val newLastChildNode = pstate.infoset.maybeLastChild
+ if (newLastChildNode.isDefined) {
+ // We have potentially added a child to to this complex during
// this array loop.
//
// If the new child is a DIArray, we know this DIArray has at
// least one element, but we don't know if we actually added a
// new one in this loop or not. So just get the last array
- // element and set it as final anyways. Note that we need a null
- // check because if we didn't add an array element this go
- // around, the last element may have already been walked and
- // freed and so could now be null.
+ // element and set it as final anyways.
//
// If it's not a DIArray, that means it's just an optional
// simple/complex and that will get set final below where all
// other non-array elements get set as final.
- newLastChildNode.get match {
- case a: DIArray => {
- val lastArrayElem = a.contents.last
- if (lastArrayElem != null) {
- lastArrayElem.setFinal()
- pstate.walker.walk()
- }
+ val lastChild = newLastChildNode.get
+ if (!lastChild.isSimple && !lastChild.isComplex) {
Review comment:
Two calls to isSimple and isComplex? Let's just add an isArray method to go with the others. It's again disappointing that x.isInstanceOf[DIArray] isn't every bit as fast as having an isArray boolean. If isSimple and isComplex are virtual functions defined on DINode and DISimple and DIComplex, then I fail to understand why they wouldn't be pretty much identical in preformance to x.isInstanceOf, why would this be?
This really suggests we should have an Int member which we initialize to an integer enum value that identifies simple/complex/array, and then we can have inline final def isSimple = nodeKindEnum eq SIMPLE and similar for isComplex and isArray. These would then not be virtual functions at all. But I don't like having to play that sort of tricks. We're just reinventing virtual dispatch, doing so in a less general way that we know will just turn into an integer equality test.
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/infoset/InfosetWalker.scala
##########
@@ -201,26 +229,60 @@ class InfosetWalker private (
* possible.
*
* It is an error to call walk() if isFinished returns true
+ *
+ * For performance reasons, sometimes a call to walk will intentionally not
+ * try to take any steps even if it is possible. This lets us essentially
+ * batch infoset events and improve performance. If this is the last time
+ * walk() will be called, the lastWalk parameter should be set to true, which
+ * will cause walk() to not skip any steps.
*/
- def walk(): Unit = {
+ def walk(lastWalk: Boolean = false): Unit = {
Assert.usage(!finished)
- var maybeNextStep: Maybe[InfosetWalkerStep] = Nope
- while ({
- maybeNextStep = getNextStep()
- maybeNextStep.isDefined
- }) {
- maybeNextStep.get.step()
+ if (walkSkipRemaining > 0 && !lastWalk) {
Review comment:
I don't understand why this improves performance.
Every node has to eventually be walked. Are we cutting down the number of times walk is called only to return without being able to make progress? So this design is effectively polling and what we're doing is just to cut down on the polling frequency?
If so, then it suggests to me we should only be calling walk() when a there is no outstanding PoU and only after N elements are added to the infoset, for some fairly large N (like 100). But perhaps this algorithm achieves that same thing?
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/infoset/InfosetWalker.scala
##########
@@ -178,7 +179,11 @@ class InfosetWalker private (
* we increment this value on the top of the stack so that the starting index
* is correct.
*/
- private var containerNode: DINode = startingContainerNode
+ private var containerNodeStack: MStackOf[DINode] = {
Review comment:
Really? Maintaining this whole separate stack is faster than just doing a test to see if enclosing parent is a DIArray and indirecting past that to the enclosing DIComplex if so? That's kind of disappointing. Maybe we should just have a DINode.nodeKind : Int, where 0 is DISimple, 1 is DIComplex, 2 is DIArray, etc. and then containerNode would just be { if (nodeKind == 2) parent.parent else parent }
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/infoset/InfosetWalker.scala
##########
@@ -321,132 +390,114 @@ class InfosetWalker private (
}
}
- private trait InfosetWalkerStep {
- /**
- * Output events associated with this step kind, and mutate the
- * InfosetWalker state to walk to the next node in the infoset
- */
- def step(): Unit
-
- final def moveToFirstChild(newContainer: DINode): Unit = {
- containerNode = newContainer
- containerIndexStack.push(0)
- }
+ @inline
+ private def moveToFirstChild(newContainer: DINode): Unit = {
+ containerNodeStack.push(newContainer)
+ containerIndexStack.push(0)
+ }
- final def moveToContainer(): Unit = {
- containerNode = containerNode.containerNode
- containerIndexStack.pop
- }
+ @inline
+ private def moveToContainer(): Unit = {
+ containerNodeStack.pop
+ containerIndexStack.pop
+ }
- final def moveToNextSibling(): Unit = {
- containerIndexStack.push(containerIndexStack.pop + 1)
- }
+ @inline
+ private def moveToNextSibling(): Unit = {
+ val top = containerIndexStack.top
+ containerIndexStack.setTop(top + 1)
}
- private object InfosetWalkerStepStart extends InfosetWalkerStep {
- /**
- * Start the document. Note that because the top of container index is
- * initialized to one less that the starting index, we also call
- * moveToNextSibling to increment the starting index to the correct
- * position.
- */
- override def step(): Unit = {
- outputter.startDocument()
- moveToNextSibling()
- }
+ /**
+ * Start the document. Note that because the top of container index is
+ * initialized to one less that the starting index, we also call
+ * moveToNextSibling to increment the starting index to the correct
+ * position.
+ */
+ private def infosetWalkerStepStart(): Unit = {
+ outputter.startDocument()
+ moveToNextSibling()
}
- private object InfosetWalkerStepEnd extends InfosetWalkerStep {
- /**
- * End document and clean up state. Setting finished to true causes
- * the next step to be None, walk() will return, and the caller
- * should not call walk() again because it is finished.
- */
- override def step(): Unit = {
- outputter.endDocument()
- containerNode = null
- containerIndexStack = null
- finished = true
- }
+ /**
+ * End document and clean up state. Setting finished to true causes
+ * the next step to be None, walk() will return, and the caller
+ * should not call walk() again because it is finished.
+ */
+ private def infosetWalkerStepEnd(): Unit = {
+ outputter.endDocument()
+ containerNodeStack = null
+ containerIndexStack = null
+ finished = true
}
- private object InfosetWalkerStepMove extends InfosetWalkerStep {
- /**
- * Output start/end events for DIComplex/DIArray/DISimple, and mutate state
- * so we are looking at the next node in the infoset.
- */
- def step(): Unit = {
- val children = containerNode.contents
- val childIndex = containerIndexStack.top
-
- if (childIndex < children.size) {
- // This block means we need to create a start event for the element in
- // the children array at childIndex. We then need to mutate the walker
- // state so the next call to step() is for either the first child of
- // this element or the next sibling.
-
- children(childIndex) match {
- case s: DISimple => {
- if (!s.isHidden || walkHidden) {
- outputter.startSimple(s)
- outputter.endSimple(s)
- }
- if (removeUnneeded) {
- // now we can remove this simple element to free up memory
- containerNode.freeChildIfNoLongerNeeded(containerIndexStack.top)
- }
- moveToNextSibling()
- }
- case c: DIComplex => {
- if (!c.isHidden || walkHidden) {
- outputter.startComplex(c)
- moveToFirstChild(c)
- } else {
- moveToNextSibling()
- }
- }
- case a: DIArray => {
- if (!a.isHidden || walkHidden) {
- outputter.startArray(a)
- moveToFirstChild(a)
- } else {
- moveToNextSibling()
- }
- }
- }
+ /**
+ * Output start/end events for DIComplex/DIArray/DISimple, and mutate state
+ * so we are looking at the next node in the infoset.
+ */
+ private def infosetWalkerStepMove(containerNode: DINode, containerIndex: Int): Unit = {
+ val children = containerNode.contents
- } else {
- // This else block means that we incremented the index stack past the
- // number of children in this container (must be a DIComplex/DIDocument
- // or DIArray), which means we have created events for all if its
- // children. So we must now create the appropriate end event for the
- // container and then mutate the state so that we are looking at its
- // next sibling. Note that if there is no next sibling, that will be
- // found the next time step() is called (because we incremented past
- // this element) and we will fall into this block again, call the end
- // function again, and repeat the process.
-
- // create appropriate end event
- containerNode match {
- case a: DIArray => {
- outputter.endArray(a)
- }
- case c: DIComplex => {
- outputter.endComplex(c)
- }
- case _ => Assert.impossible()
- }
+ if (containerIndex < children.size) {
+ // This block means we need to create a start event for the element in
+ // the children array at containerIndex. We then need to mutate the walker
+ // state so the next call to step() is for either the first child of
+ // this element or the next sibling.
- // we've ended this array/complex associated with the container, so we
- // now want to move to the parent container, potentially free up the
- // memory associated with this container, and then move to the next
- // sibling of this container
- moveToContainer()
+ val child = children(containerIndex)
+
+ if (child.isSimple) {
+ if (!child.isHidden || walkHidden) {
+ val simple = child.asInstanceOf[DISimple]
+ outputter.startSimple(simple)
Review comment:
We could combine these two virtual functions into one, or provide a combined variant startEndSimple(simple). That would cut two virtual function calls down to one.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-daffodil] stevedlawrence commented on a change in pull request #419: Reduce performance degradation caused by commit b0f59ef7c8
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #419:
URL: https://github.com/apache/incubator-daffodil/pull/419#discussion_r488066492
##########
File path: daffodil-runtime1-unparser/src/main/scala/org/apache/daffodil/processors/unparsers/ElementUnparser.scala
##########
@@ -477,7 +477,7 @@ sealed trait RegularElementUnparserStartEndStrategy
}
val cur = state.currentInfosetNode
if (cur.isComplex)
- cur.asComplex.setFinal()
+ cur.isFinal = true
Review comment:
I think it's actually both a mixture of removeing the trait, and making things publicly modifable. For example, with a trait we get something like this:
```
public class WithFinalTrait implements Finalizable {
private boolean Finalizable$$_isFinal;
public void Finalizable$$_isFinal_$eq(boolean);
Code:
0: aload_0
1: iload_1
2: putfield #15 // Field Finalizable$$_isFinal:Z
5: return
public void setFinal();
Code:
0: aload_0
1: invokestatic #27 // Method Finalizable$class.setFinal:(LFinalizable;)V
4: return
}
public abstract class Finalizable$class {
public static void setFinal(Finalizable);
Code:
0: aload_0
1: iconst_1
2: invokeinterface #13, 2 // InterfaceMethod Finalizable.Finalizable$$_isFinal_$eq:(Z)V
7: return
}
```
So to set the flag we first call setFinal, which calls the static setFinal method on Finalizable, which then calls \_isFinal_$eq, which finaly sets the variable to set the value. So three function calls: setFinal, static setFinal, and isFinal_$eq.
Without the trait, but with private _isFinal, the bytecode is this:
```
public class WithOutFinalTrait {
private boolean _isFinal;
private void _isFinal_$eq(boolean);
Code:
0: aload_0
1: iload_1
2: putfield #13 // Field _isFinal:Z
5: return
public void setFinal();
Code:
0: aload_0
1: iconst_1
2: invokespecial #22 // Method _isFinal_$eq:(Z)V
5: return
}
```
So a call to setFinal directly calls _isFinal_$eq to set the variable. So we remove the one static setFinal call.
With a public final flag and direct modification, the bytecode looks like this:
```
public class PublicIsFinal {
private boolean _isFinal;
public boolean _isFinal();
Code:
0: aload_0
1: getfield #13 // Field _isFinal:Z
4: ireturn
public void _isFinal_$eq(boolean);
Code:
0: aload_0
1: iload_1
2: putfield #13 // Field _isFinal:Z
5: return
}
```
So _isFinal_$eq is called directly which modifies the field directly. So now we're down to a single function call.
So we go from three functions calls with a trait, to one function call with public member. This feels like it shouldn't make that much of a difference, but it definitely did. It wasn't huge, and I'm not sure I would reccommend we do this everywhere because it does make readability/reusability more difficult, but in performance critical situations, avoiding traits and private vars seems to help a bit.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-daffodil] stevedlawrence commented on a change in pull request #419: Reduce performance degradation caused by commit b0f59ef7c8
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #419:
URL: https://github.com/apache/incubator-daffodil/pull/419#discussion_r488074239
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/infoset/InfosetWalker.scala
##########
@@ -321,132 +390,114 @@ class InfosetWalker private (
}
}
- private trait InfosetWalkerStep {
- /**
- * Output events associated with this step kind, and mutate the
- * InfosetWalker state to walk to the next node in the infoset
- */
- def step(): Unit
-
- final def moveToFirstChild(newContainer: DINode): Unit = {
- containerNode = newContainer
- containerIndexStack.push(0)
- }
+ @inline
+ private def moveToFirstChild(newContainer: DINode): Unit = {
+ containerNodeStack.push(newContainer)
+ containerIndexStack.push(0)
+ }
- final def moveToContainer(): Unit = {
- containerNode = containerNode.containerNode
- containerIndexStack.pop
- }
+ @inline
+ private def moveToContainer(): Unit = {
+ containerNodeStack.pop
+ containerIndexStack.pop
+ }
- final def moveToNextSibling(): Unit = {
- containerIndexStack.push(containerIndexStack.pop + 1)
- }
+ @inline
+ private def moveToNextSibling(): Unit = {
+ val top = containerIndexStack.top
+ containerIndexStack.setTop(top + 1)
}
- private object InfosetWalkerStepStart extends InfosetWalkerStep {
- /**
- * Start the document. Note that because the top of container index is
- * initialized to one less that the starting index, we also call
- * moveToNextSibling to increment the starting index to the correct
- * position.
- */
- override def step(): Unit = {
- outputter.startDocument()
- moveToNextSibling()
- }
+ /**
+ * Start the document. Note that because the top of container index is
+ * initialized to one less that the starting index, we also call
+ * moveToNextSibling to increment the starting index to the correct
+ * position.
+ */
+ private def infosetWalkerStepStart(): Unit = {
+ outputter.startDocument()
+ moveToNextSibling()
}
- private object InfosetWalkerStepEnd extends InfosetWalkerStep {
- /**
- * End document and clean up state. Setting finished to true causes
- * the next step to be None, walk() will return, and the caller
- * should not call walk() again because it is finished.
- */
- override def step(): Unit = {
- outputter.endDocument()
- containerNode = null
- containerIndexStack = null
- finished = true
- }
+ /**
+ * End document and clean up state. Setting finished to true causes
+ * the next step to be None, walk() will return, and the caller
+ * should not call walk() again because it is finished.
+ */
+ private def infosetWalkerStepEnd(): Unit = {
+ outputter.endDocument()
+ containerNodeStack = null
+ containerIndexStack = null
+ finished = true
}
- private object InfosetWalkerStepMove extends InfosetWalkerStep {
- /**
- * Output start/end events for DIComplex/DIArray/DISimple, and mutate state
- * so we are looking at the next node in the infoset.
- */
- def step(): Unit = {
- val children = containerNode.contents
- val childIndex = containerIndexStack.top
-
- if (childIndex < children.size) {
- // This block means we need to create a start event for the element in
- // the children array at childIndex. We then need to mutate the walker
- // state so the next call to step() is for either the first child of
- // this element or the next sibling.
-
- children(childIndex) match {
- case s: DISimple => {
- if (!s.isHidden || walkHidden) {
- outputter.startSimple(s)
- outputter.endSimple(s)
- }
- if (removeUnneeded) {
- // now we can remove this simple element to free up memory
- containerNode.freeChildIfNoLongerNeeded(containerIndexStack.top)
- }
- moveToNextSibling()
- }
- case c: DIComplex => {
- if (!c.isHidden || walkHidden) {
- outputter.startComplex(c)
- moveToFirstChild(c)
- } else {
- moveToNextSibling()
- }
- }
- case a: DIArray => {
- if (!a.isHidden || walkHidden) {
- outputter.startArray(a)
- moveToFirstChild(a)
- } else {
- moveToNextSibling()
- }
- }
- }
+ /**
+ * Output start/end events for DIComplex/DIArray/DISimple, and mutate state
+ * so we are looking at the next node in the infoset.
+ */
+ private def infosetWalkerStepMove(containerNode: DINode, containerIndex: Int): Unit = {
+ val children = containerNode.contents
- } else {
- // This else block means that we incremented the index stack past the
- // number of children in this container (must be a DIComplex/DIDocument
- // or DIArray), which means we have created events for all if its
- // children. So we must now create the appropriate end event for the
- // container and then mutate the state so that we are looking at its
- // next sibling. Note that if there is no next sibling, that will be
- // found the next time step() is called (because we incremented past
- // this element) and we will fall into this block again, call the end
- // function again, and repeat the process.
-
- // create appropriate end event
- containerNode match {
- case a: DIArray => {
- outputter.endArray(a)
- }
- case c: DIComplex => {
- outputter.endComplex(c)
- }
- case _ => Assert.impossible()
- }
+ if (containerIndex < children.size) {
+ // This block means we need to create a start event for the element in
+ // the children array at containerIndex. We then need to mutate the walker
+ // state so the next call to step() is for either the first child of
+ // this element or the next sibling.
Review comment:
The state of the infoset walker is always pointing at a child index for elements to create events for. This block is going to create an event for the containerIndex'th element in the current containerNode. After it creates that event, it needs to mutate state so the next time we take a step we are looking at the next element. If this is a complex type, the next element is the first child. If this is not a complex element, the next element is the next sibling. So that's what this is trying to say. I'll update this to be a bit more clear.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-daffodil] stevedlawrence commented on a change in pull request #419: Reduce performance degradation caused by commit b0f59ef7c8
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #419:
URL: https://github.com/apache/incubator-daffodil/pull/419#discussion_r488066492
##########
File path: daffodil-runtime1-unparser/src/main/scala/org/apache/daffodil/processors/unparsers/ElementUnparser.scala
##########
@@ -477,7 +477,7 @@ sealed trait RegularElementUnparserStartEndStrategy
}
val cur = state.currentInfosetNode
if (cur.isComplex)
- cur.asComplex.setFinal()
+ cur.isFinal = true
Review comment:
I think it's actually both a mixture of removeing the trait, and making things publicly modifable. For example, with a trait we get something like this:
```
public class WithFinalTrait implements Finalizable {
private boolean Finalizable$$_isFinal;
public void Finalizable$$_isFinal_$eq(boolean);
Code:
0: aload_0
1: iload_1
2: putfield #15 // Field Finalizable$$_isFinal:Z
5: return
public void setFinal();
Code:
0: aload_0
1: invokestatic #27 // Method Finalizable$class.setFinal:(LFinalizable;)V
4: return
}
public abstract class Finalizable$class {
public static void setFinal(Finalizable);
Code:
0: aload_0
1: iconst_1
2: invokeinterface #13, 2 // InterfaceMethod Finalizable.Finalizable$$_isFinal_$eq:(Z)V
7: return
}
```
So to set the flag we first call setFinal, which calls the static setFinal method on Finalizable, which then calls \_isFinal_$eq, which finaly sets the variable to set the value. So three function calls: setFinal, static setFinal, and isFinal_$eq.
Without the trait, but with private _isFinal, the bytecode is this:
public class WithOutFinalTrait {
private boolean _isFinal;
private void _isFinal_$eq(boolean);
Code:
0: aload_0
1: iload_1
2: putfield #13 // Field _isFinal:Z
5: return
public void setFinal();
Code:
0: aload_0
1: iconst_1
2: invokespecial #22 // Method _isFinal_$eq:(Z)V
5: return
}
So a call to setFinal directly calls _isFinal_$eq to set the variable. So we remove the one static setFinal call.
With a public final flag and direct modification, the bytecode looks like this:
public class PublicIsFinal {
private boolean _isFinal;
public boolean _isFinal();
Code:
0: aload_0
1: getfield #13 // Field _isFinal:Z
4: ireturn
public void _isFinal_$eq(boolean);
Code:
0: aload_0
1: iload_1
2: putfield #13 // Field _isFinal:Z
5: return
}
So _isFinal_$eq is called directly which modifies the field directly. So now we're down to a single function call.
So we go from three functions calls with a trait, to one function call with public member. This feels like it shouldn't make that much of a difference, but it definitely did. It wasn't huge, and I'm not sure I would reccommend we do this everywhere because it does make readability/reusability more difficult, but in performance critical situations, avoiding traits and private vars seems to help a bit.
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-daffodil] stevedlawrence commented on a change in pull request #419: Reduce performance degradation caused by commit b0f59ef7c8
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #419:
URL: https://github.com/apache/incubator-daffodil/pull/419#discussion_r488085223
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/infoset/InfosetImpl.scala
##########
@@ -1458,6 +1454,12 @@ sealed class DIComplex(override val erd: ElementRuntimeData)
override def children = childNodes.toStream
+ override def maybeLastChild: Maybe[DINode] = {
+ val len = childNodes.length
+ if (len > 0) Maybe(childNodes(len - 1))
Review comment:
Will do
----------------------------------------------------------------
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:
users@infra.apache.org
[GitHub] [incubator-daffodil] stevedlawrence commented on a change in pull request #419: Reduce performance degradation caused by commit b0f59ef7c8
Posted by GitBox <gi...@apache.org>.
stevedlawrence commented on a change in pull request #419:
URL: https://github.com/apache/incubator-daffodil/pull/419#discussion_r488077124
##########
File path: daffodil-runtime1/src/main/scala/org/apache/daffodil/infoset/InfosetWalker.scala
##########
@@ -321,132 +390,114 @@ class InfosetWalker private (
}
}
- private trait InfosetWalkerStep {
- /**
- * Output events associated with this step kind, and mutate the
- * InfosetWalker state to walk to the next node in the infoset
- */
- def step(): Unit
-
- final def moveToFirstChild(newContainer: DINode): Unit = {
- containerNode = newContainer
- containerIndexStack.push(0)
- }
+ @inline
+ private def moveToFirstChild(newContainer: DINode): Unit = {
+ containerNodeStack.push(newContainer)
+ containerIndexStack.push(0)
+ }
- final def moveToContainer(): Unit = {
- containerNode = containerNode.containerNode
- containerIndexStack.pop
- }
+ @inline
+ private def moveToContainer(): Unit = {
+ containerNodeStack.pop
+ containerIndexStack.pop
+ }
- final def moveToNextSibling(): Unit = {
- containerIndexStack.push(containerIndexStack.pop + 1)
- }
+ @inline
+ private def moveToNextSibling(): Unit = {
+ val top = containerIndexStack.top
+ containerIndexStack.setTop(top + 1)
}
- private object InfosetWalkerStepStart extends InfosetWalkerStep {
- /**
- * Start the document. Note that because the top of container index is
- * initialized to one less that the starting index, we also call
- * moveToNextSibling to increment the starting index to the correct
- * position.
- */
- override def step(): Unit = {
- outputter.startDocument()
- moveToNextSibling()
- }
+ /**
+ * Start the document. Note that because the top of container index is
+ * initialized to one less that the starting index, we also call
+ * moveToNextSibling to increment the starting index to the correct
+ * position.
+ */
+ private def infosetWalkerStepStart(): Unit = {
+ outputter.startDocument()
+ moveToNextSibling()
}
- private object InfosetWalkerStepEnd extends InfosetWalkerStep {
- /**
- * End document and clean up state. Setting finished to true causes
- * the next step to be None, walk() will return, and the caller
- * should not call walk() again because it is finished.
- */
- override def step(): Unit = {
- outputter.endDocument()
- containerNode = null
- containerIndexStack = null
- finished = true
- }
+ /**
+ * End document and clean up state. Setting finished to true causes
+ * the next step to be None, walk() will return, and the caller
+ * should not call walk() again because it is finished.
+ */
+ private def infosetWalkerStepEnd(): Unit = {
+ outputter.endDocument()
+ containerNodeStack = null
+ containerIndexStack = null
+ finished = true
}
- private object InfosetWalkerStepMove extends InfosetWalkerStep {
- /**
- * Output start/end events for DIComplex/DIArray/DISimple, and mutate state
- * so we are looking at the next node in the infoset.
- */
- def step(): Unit = {
- val children = containerNode.contents
- val childIndex = containerIndexStack.top
-
- if (childIndex < children.size) {
- // This block means we need to create a start event for the element in
- // the children array at childIndex. We then need to mutate the walker
- // state so the next call to step() is for either the first child of
- // this element or the next sibling.
-
- children(childIndex) match {
- case s: DISimple => {
- if (!s.isHidden || walkHidden) {
- outputter.startSimple(s)
- outputter.endSimple(s)
- }
- if (removeUnneeded) {
- // now we can remove this simple element to free up memory
- containerNode.freeChildIfNoLongerNeeded(containerIndexStack.top)
- }
- moveToNextSibling()
- }
- case c: DIComplex => {
- if (!c.isHidden || walkHidden) {
- outputter.startComplex(c)
- moveToFirstChild(c)
- } else {
- moveToNextSibling()
- }
- }
- case a: DIArray => {
- if (!a.isHidden || walkHidden) {
- outputter.startArray(a)
- moveToFirstChild(a)
- } else {
- moveToNextSibling()
- }
- }
- }
+ /**
+ * Output start/end events for DIComplex/DIArray/DISimple, and mutate state
+ * so we are looking at the next node in the infoset.
+ */
+ private def infosetWalkerStepMove(containerNode: DINode, containerIndex: Int): Unit = {
+ val children = containerNode.contents
- } else {
- // This else block means that we incremented the index stack past the
- // number of children in this container (must be a DIComplex/DIDocument
- // or DIArray), which means we have created events for all if its
- // children. So we must now create the appropriate end event for the
- // container and then mutate the state so that we are looking at its
- // next sibling. Note that if there is no next sibling, that will be
- // found the next time step() is called (because we incremented past
- // this element) and we will fall into this block again, call the end
- // function again, and repeat the process.
-
- // create appropriate end event
- containerNode match {
- case a: DIArray => {
- outputter.endArray(a)
- }
- case c: DIComplex => {
- outputter.endComplex(c)
- }
- case _ => Assert.impossible()
- }
+ if (containerIndex < children.size) {
+ // This block means we need to create a start event for the element in
+ // the children array at containerIndex. We then need to mutate the walker
+ // state so the next call to step() is for either the first child of
+ // this element or the next sibling.
- // we've ended this array/complex associated with the container, so we
- // now want to move to the parent container, potentially free up the
- // memory associated with this container, and then move to the next
- // sibling of this container
- moveToContainer()
+ val child = children(containerIndex)
+
+ if (child.isSimple) {
+ if (!child.isHidden || walkHidden) {
+ val simple = child.asInstanceOf[DISimple]
+ outputter.startSimple(simple)
Review comment:
I actually tried that change and it seemed to not make a difference in this case.
----------------------------------------------------------------
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:
users@infra.apache.org