Add a build/write-prefetch unparse path driven by a dedicated Builder tree by olabusayoT · Pull Request #1736 · apache/daffodil

olabusayoT · 2026-09-29T22:56:48Z

Adds an off-by-default (useBuildWritePrefetch) two-pass unparse path in
which build races ahead of write over the same infoset tree, resolving
OVCs directly against the tree instead of through Suspensions where
possible.

Build is driven by a Builder tree paralleling the Unparser tree and wired
through the grammar combinators, with per-parse BuildFrames on an
explicit BuildCursor stack so building can stop after any step and resume
later. Write pulls build forward through awaitChild and
childExistsOrFinal until unparsePrefetchWindowNodes are built ahead or
pending suspensions exceed unparsePendingSuspensionTripLimit, all on the
calling thread.

The Builder tree is only constructed when the tunable is on and
hasAnyPrefetchBeneficialOVC holds, which is scoped per compiling root
and requires a non-constant OVC.

writeContent and unparse share their setup, dispatch and teardown through
per-combinator run methods, WriteUnparser.dispatchBody and
runElementContent, so SpecifiedLengthPrefixedUnparser now also resolves
its prefix length when the body throws during unparse.

DaffodilTunables.withTunable is generated as withTunablePart0..N to stay
under the JVM method size limit.

DAFFODIL-3065

Design: https://cwiki.apache.org/confluence/spaces/DAFFODIL/pages/451975094/Build+Write-Prefetch+Unparse+Path

stevedlawrence

checkpoint comments

stevedlawrence · 2026-09-30T11:58:40Z

+ * explicit stack, so building can stop after any step and continue later
+ * without holding a thread or a JVM call stack.
+ */
+trait Builder extends Serializable {


Should we rename these classes to make it more clear they are about building our internal infoset? Otherwise being in the unparser namespace one might assume they are about building unparsers or something. For example, maybe this wants to be InfosetBuilder.

Might additionally want to move it to the infoset package as well so it lives along side the InfosetInputter stuff sine the two are very closely related.

stevedlawrence · 2026-09-30T12:04:03Z

+   * the Unparser tree. Most Grams create or select no infoset content and
+   * inherit this Nope default; only those that do override it.
+   */
+  def builder: Maybe[Builder] = Maybe.Nope


Should we follow the patter of Parsers and Unparsers and change this to return a Builder instead of a Maybe[Builder]. Primitives that do not return a builder can return a NadaBuilder (similar to NadaParser and NadaUnparser, which should get optimized out?

I'm honestly not sure if there is a reason for NadaParser/Unaprsers (maybe to avoid a bunch of One/Some's, but maybe there's value in consistency?

It might be worth considering if there's value in removing NadaParser/Unprsers and changing it all to Maybe.s It will then at least force primitives to consider how to handle parsers. Right now they don't necessarily have to do that and we just throw an abort it a NadaParser/Unparse ends up not being optimized out.

Added DAFFODIL-3103 to track replacement of Nada* objects with Maybe[Object], for now will replace Builder with NadaBuilder

stevedlawrence · 2026-09-30T12:15:33Z

+      eValue.builder
+    }
+  }
+  private lazy val eReptypeBuilder: Maybe[Builder] = repTypeElementGram.builder


Suggest we name this eRepTypeBuilder (with a capital T) to match the reptype parser/unparser converntion.

stevedlawrence · 2026-09-30T12:23:52Z

+
+  // Shares the memoized unparser above for unparseBegin/unparseEnd, so
+  // build and write see identical node-creation behavior.
+  override lazy val builder: Maybe[Builder] = unparser match {


This might read a bit better if we change the logic in val unparser to something like

private lazy val isSomethingAboutSpecifiedLength = ... val unparser = if (context.isOutputValueCalc) { new ElementOCVSpecifiedLengthUnparser(...) } else if (isSomethingAboutSpecifiedLength) { new ElementSpecifiedLengthUnparser(...) } else { subComb.unparser }

And then this builder can have very similar same conditionals just with Builders instead of Unparsers. eg.

val builder = if (context.isOUtputValueCalc || isSomethignAboutSpecifiedLength) { ElementBuidler(...) } else { subComb.builder }

That way it's very clear that the logic is basically the same rather than relying on type matching on parsers.

stevedlawrence · 2026-09-30T12:34:24Z

+  // Shares the memoized unparser above for unparseBegin/unparseEnd, so
+  // build and write see identical nilled/OVC/IVC node-creation behavior.
+  override lazy val builder: Maybe[Builder] = {
+    val eu = unparser.asInstanceOf[ElementUnparserBase]


Same here, I would rather see the same logic rather than type matching. It's much easier to visually verify the conditionalsare the same rather than figuring out which Unparsers are created.

This one doesn't pattern match but uses the elementUnparser to call unparseBegin/End

stevedlawrence · 2026-09-30T13:45:15Z

+    writeState.getDataOutputStream.setFinished(writeState)
+
+    (singlePassBytes, walkerOut.toByteArray)
+  }


These tests seem like they are using too much of our internals for a generic TestUtil function. This feels much more like unit tests specific to testing that prefetching is working as expected. I'd prefer if we dont' invent another test infrasture for this (I found AI really likes to create it's own test frameworks) so if there's an alternative (like TDML tests) I'd prefer that. But if we really do need this, suggest it becomes it's own file specific to testing prefetch.

stevedlawrence · 2026-09-30T13:45:48Z

+   * Off by default: growing past initialSize isn't itself wrong, so paying
+   * this bookkeeping cost on every push isn't worth it normally.
+   */
+  final val trackMaxSizeReached: Boolean = false


Can you split mstack changes to a new PR?

Note that this is a prime example to avoid combine unrelated changes in PR, especially when they are complex. I believe I looked at the mstack changes as part of another PR and thought it looked reasonable, but we ended up closing that PR and now it needs to be reviewed again.

stevedlawrence · 2026-09-30T14:16:21Z


+  /**
+   * True if a component reachable from this element has a
+   * dfdl:outputValueCalc resolvable without writing; gates


Again, let's avoid the term "writing", it's not a term we've traditionally used except for actually writing bits and could cause confusion.

stevedlawrence · 2026-09-30T14:19:02Z

+  // Not forced eagerly: onPath only references this when
+  // tunable.useBuildWritePrefetch is on (the tunable is fixed at compile
+  // time), so schemas that never enable it never pay to construct the
+  // Builder tree.


Doe sthis increase compilation time very much? If not, I might suggest we always create the builder, regardless ofthe tunable. That way the tunable could be dynamically changed at unparse time without needing to recompile. I'm not sure that would actually be used in practice, but it could be useful for performance testing. E.g. compile once, run once with tunable true, run once with tunable false, and then you have a good idea if a certain value is significantly better. We can still compile a DataProcessor with a specific value, but it means it can be overridden.

stevedlawrence · 2026-09-30T14:21:38Z


  final def freeChildIfNoLongerNeeded(index: Int, doFree: Boolean): Unit = {
    val node = _contents(index)
+    // A null slot means write already freed it before build's redundant


s/write/unparse/

Also, I'm not sure I understand this comment or why this change is needed. Seems like the builder should never call this function since all it's doing is creating nodes, and skimming the code it's not immediately obvious that it ever does call this. Seems only the unparse() function should free these nodes after it has unparsed them. Same with the below change in this file.

stevedlawrence

checkpoint comments

stevedlawrence · 2026-10-02T17:58:58Z

+ * against a `BuildState`, never a write-side `UState`.
+ */
+final class BuildCursor(root: Builder, val state: UState, ctx: UnparseSharedContext) {
+  private var stack = new Array[BuildFrame](32)


Does this want to be an MStack? Seems like this does things very similar to what an MStack is used for and some of the things like depth/push/pop/etc you get for free.

stevedlawrence · 2026-10-02T18:03:00Z

+final class BuildCursor(root: Builder, val state: UState, ctx: UnparseSharedContext) {
+  private var stack = new Array[BuildFrame](32)
+  private var depth = 0
+  private var failure: Throwable = null


Suggest we drop this for simplicity? I assume once we throw a BuildAbortedException we'll immediately turn that into an UnparseError or something and terminate the unparse so there shouldn't be a concern about calling back into this.

stevedlawrence · 2026-10-02T18:05:37Z

+   */
+  def advance(): Unit = {
+    if (failure != null) {
+      throw new BuildAbortedException(failure)


If we do keep the failure var, does this want to be an assert? It feels like calling the builder after it's thrown a exception should be considered a usage error.

stevedlawrence · 2026-10-02T18:08:17Z

+   * The lead only rises as build adds nodes and only falls as write
+   * finishes them, so this is the peak over a whole run.
+   */
+  def peakLead: Long = peakLead_


This doesn't seem to be used, is this for diagnostics?

stevedlawrence · 2026-10-02T18:32:44Z

+          lead = newLead
+          if (
+            ctx.leadExceedsPrefetchLimit ||
+            ctx.suspensionTracker.pendingCount > ctx.pendingSuspensionTripLimit


Doesn't this mean that if we have a lot of suspensions (more than the trip limit) that we stop trying to build the infoset, and from that point on we'll only build elements one at a time until the suspensions go down? Could switching to one at a time building going to cause performance issues?

I feel like we actually want the opposite behavior: if we have a bunch of suspensions, then maybe we consider increasing the prefetch limit hoping to read some more elements that unblock some of those existing suspensions. I'm not sure we really need that since unparse() will continue to make progress and eventually get there. Also the default value of 500 feels pretty high, I wonder if we're never hitting hitting that limit and so aren't finding out what happens when this is hit?

If we aren't actually hitting this, I might suggest we remove the suspensionTripLimit. A format with lots of suspensions could always increase the prefetch limit which should have a similar effect.

stevedlawrence · 2026-10-02T19:07:19Z

+    private final val NextChild = 0
+    private final val AfterScalar = 1
+    private final val InArray = 2
+    private final val AfterOccurrence = 3


Might put this in an object SequenceBuilder to avoid allocating these vals for every new frame. It's only 4 vals so probably not a big deal, but the less memory we allocate the better.

stevedlawrence · 2026-10-02T19:20:10Z

+ * `getDataOutputStream` is NOT stubbed: generic `UState` utility methods
+ * (toString, currentLocation, bitPos0b) call into it, so `BuildState`
+ * lazily constructs an actual DOS wrapping a no-op sink purely to satisfy
+ * that.


I would think if something is calling getDataOutputStream on a BuildState that's a usage error and it should assert. Things like bitPosotion/currentLocation/etc are pretty meaningless in a BuildState so if someting has access to a BuildState and queries those it's probably a bug.

stevedlawrence · 2026-10-02T19:23:08Z

+final class BuildState(
+  private val inputter: InfosetInputter,
+  sharedCtx: UnparseSharedContext,
+  diagnosticsArg: Seq[api.Diagnostic],


Our usages always pass Nil to the diagnosticsArg. I woudl assume any diagnostics that a Builder/BuildState create would bubbble up to whatever called build and add the diagnostics to the real Ustate?

stevedlawrence · 2026-10-02T19:25:33Z

+    sharedCtx.tunable,
+    areDebugging
+  )
+  with SuspensionCapableUState


Is this really suspension capable? I wouldn't think the builder could create suspensios since it doesn't ever call unparse() which I think is the only thing that ever creates suspensions?

stevedlawrence · 2026-10-02T19:29:50Z

+  }
+
+  final override def documentElement: DIDocument = inputter.documentElement
+}


Feels like we might want to rethink this BuildState. Wonder if it wants to be it's own thing that UState mixes in, and the functions that work on both a BuildState and a UState just accept a BuildState.

stevedlawrence

checkpoint review

stevedlawrence · 2026-10-05T17:57:00Z

  val traceIter = Seq(
    "set infosetParents 1",
    "display info parser",
+    "display info unparser",


I'm not sure we want this change. It's kindof unrelated to this PR, and it will cause "no unparser available" when parsing. Maybe what we really need is an new info processor command or something that will inspect the state and output either the parser or unparser depending on the state. We should maybe even just deprecate info parser/unparser and have them call into info processor, users shouldn't have to worry about it we're parsing or unparsing.

Created DAFFODIL-3104 to track

stevedlawrence · 2026-10-05T19:41:56Z

+   * propagates to the suspension machinery, which retries it later. It is
+   * inline so the by-name body costs no closure per call.
+   */
+  inline final def withRetryIfBlocking[T](inline body: => T): T = body


It looks like withRetryIfBlocking doesn't do anything. At one point we had logic to try to resume the expressions where it was last blocked, but commit e479e92 removed that with the assumption that most expressions are small and so can just be reevaluated from the beginning. And I believe our coroutines have overhead so it's possible that overhead isn't even any better than evaluating from the beginning, especially with small expressions.

https://github.com/apache/daffodil/blob/main/daffodil-core/src/main/scala/org/apache/daffodil/runtime1/dpath/DState.scala#L146

I would suggest rather than removing withRetryIfBlocking we comment it out for now, and reference the linked comment above. If we eventually do want to remove it, we should remove everything including this function and the commented code.

Sounds good; the old code (removed here, will update #1744 to comment it out instead) created closures that were showing up in JFR

stevedlawrence · 2026-10-06T12:19:37Z

+  // The bounds do not depend on the state, so a caller with no
+  // ParseOrUnparseState can read them directly.
+  final def minRepeatsFixed: Long = minRepeats_
+  final def maxRepeatsFixed: Long = maxRepeats_


Thoughts on just making our max/minRepeats_ vals no longer private? I don't think the new functions add much but another level on indirection and a better name. Maybe rename the existing values something like min/maxRepeatsConst to make it clear that should only be used when state is not needed (e.g. unparse, most parsers except for OCK=expression). I don't dislike "fixed", but it I wonder if it could be confused with the "fixed" XSD attribute.

stevedlawrence · 2026-10-06T12:44:20Z

+  def evalSuspensions(isFinal: Boolean): Unit = {
+    // A debugger needs suspensions to resolve at the steps a single-pass
+    // unparse would resolve them, which only unparseTree's sweeps give it.
+    if (!areDebugging) sharedCtx.suspensionTracker.evalBuildResolvableSuspensions()


I'm curious if resolving suspensions specific to build made a difference? The goal of this prefetch design is to minimize even creating these build resolvable suspensions, so I would expect that there should be relatively few of these. Feels like this is more likely to scan the suspensions and not actually find anything it can resolve and just add additional overhead.

And if there are suspensions, maybe we just leave itup to normal suspension tracking to eventually kick in resolve things.

stevedlawrence · 2026-10-06T12:55:30Z

+  // An error raised by code shared with build has no data location, so it is
+  // reported at the unparse state's current location.
+  private def atUnparseLocation(ue: UnparseError, state: UState): UnparseError =
+    if (ue.getDataLocations.isEmpty) ue.withDataLocation(One(state.currentLocation)) else ue


Is it possible this could end up with confusing diagnostics? For example, we might have a diagnostic that says we are at data location 0 but the original error could have been about some element at the very end of the infoset. Is it better to just not provide a data location? It is a Maybe so this does kind of feel like a case where a DataLoation is meaningless, especially since the error would be about the infoset being broken in the prefetch.

stevedlawrence · 2026-10-06T17:12:31Z

+    }
+
+    try {
+      inputter.initialize(ssrd.elementRuntimeData, tunables)


Can this be moved to the other try catch? Looks like it's basically the same catch block.

stevedlawrence · 2026-10-06T17:14:55Z

+    } catch {
+      // Build has no state of its own to report against, so its failure is
+      // reported against unparseTree's.
+      case b: BuildAbortedException => unparseErrorResult(unparseTreeState, b.getCause)


Can we move BuildAbortedException to unparseErrorResult? I wonder if BuildAbortedException doesn't even need to exist and just wantes to be a UE? It feels like it's no different than what the single pass will do when it detects a problem in the infoset which I think is just an UnparseError.

stevedlawrence · 2026-10-06T17:16:12Z

+      finishBuildSide(infosetBuildState, rootUnparser)
+      finishUnparseSide(unparseTreeState, rootUnparser)


I think this functions want a name that makes it clear they are about the prefetch/nonstreaming stuff.

stevedlawrence · 2026-10-06T17:33:25Z

+  // shared cache behind the event factories is a contended lock across
+  // threads.
+  lazy val choiceBranchStartEvent: ChoiceBranchStartEvent = ChoiceBranchStartEvent(namedQName)
+  lazy val choiceBranchEndEvent: ChoiceBranchEndEvent = ChoiceBranchEndEvent(namedQName)


Makes me wonder if we should remove this shared choice branch cache entirely if it's causing contention? Feels odd to have choice branch event keys living on runtime data.

Maybe an alternative approach is instead of having a single ChoiceBranchMap that contains both start and end events we could have two maps, one for each kind of event. And the maps would key off of the NamedQName instead of an event, which we already have and don't need a lookup/allocation. So the choice combinator becomes something like:

class ChoiceCombinatorUnpaser( val choiceBranchStartMap: Map[NamedQName, Unparser] val choiceBranchEndMap: Map[NamedQName, Unparser] ... ) { def unparse(...) = { ... val choiceBranchMap = if (e.isStart) choiceBranchStartMap else choiceBranchEndMap val childUnparser = chocieBranchMap.get(e.erd.namedQName) ... } }

It's maybe a bit more memory having two maps, but choices are relatively rare and I imagine in most cases the choiceBranchEndMap is going to be empty since end events only exist in the map when choices have empty/hidden branches.

stevedlawrence · 2026-10-06T17:38:17Z

+   * sweep.
+   */
+  def evalBuildResolvableSuspensions(): Unit =
+    evalSuspensionsThrottled(buildResolvableOnly = true)


Related to other comments, I wonder if this actually provides value. Maybe it woudlbe best to remove it and we can add it as a future optimization if needed? It seems to add a bit of complexity to already very large change.

jadams-tresys

+1 with a few questions/possible improvements.

jadams-tresys · 2026-10-08T19:01:17Z

+      # Build ahead is the default, so run the suite once more event driven to
+      # keep that unparse path tested.
+      - name: Run Unit Tests (event-driven unparse)
+        if: matrix.os == 'ubuntu-22.04' && matrix.java_version == '17'
+        env:
+          DAFFODIL_TDML_TUNABLES: infosetBuilderMode=eventDriven
+        run: $SBT test
+


This only effects the TDML tests. Should we be doing something similar for the Scala unit tests so that we test eventDriven with those as well?

jadams-tresys · 2026-10-09T12:23:40Z

-              One(state.currentLocation),
+              Nope,


Are we losing much with this error by not including the location anymore?

We didn't lose much since it didn't really give much location info when I tested it. It's an easy enough fix tho!

jadams-tresys · 2026-10-09T12:24:31Z

-            One(state.currentLocation),
+            Nope,


Same here. Probably easy enough to find in the schema given we have the element name, but still would probably be nice to have the line info.

jadams-tresys · 2026-10-09T12:32:31Z

+      // A signal from waiting on build, not a failure of the layer; rewrapping
+      // it would hide the stall diagnostic.
+      case e: ChildNotBuiltException => throw e


Not sure how I feel about using exceptions as a signal for control flow and a little unsure how ChildNotBuiltException is being used. Is it a recoverable error? Reading the comments and seeing where it gets caught in DataProcessor.scala makes it seem like it's always just ignored.

You're right. It was not a reciverable error and wasn't ever doing anything when caught. I changed it to a regular unparseError.

olabusayoT · 2026-10-09T21:01:59Z

FYI this commit depends on #1753 , #1745 , #1744 being merged to make the PR a little easier to review

Unparse can now build the infoset tree ahead of the unparse. A tree of InfosetBuilders, made when the schema is compiled, is stepped by an InfosetBuildCursor that reads the inputter's events and attaches nodes, running up to unparseBuildAheadWindowNodes ahead of the unparse. The unparse reads the built tree as the same start and end events an inputter would produce, through TreeEventState, so it runs the unparsers that event driven unparse runs. A forward reference is resolved against the tree where it exists instead of through a suspension. The tunable infosetBuilderMode selects buildAhead, the default, or eventDriven, the previous behavior, and CI runs the unit tests a second time under eventDriven. Choice branches are keyed by NamedQName, and unparse errors raised by code shared with build carry no data location. When debugging, the infoset commands show only the nodes the unparse has reached. DAFFODIL-3065

olabusayoT requested review from jadams-tresys and stevedlawrence September 29, 2026 22:56

olabusayoT mentioned this pull request Sep 29, 2026

Add a build/write-prefetch unparse path driven by a dedicated Builder tree #1735

Closed

stevedlawrence reviewed Oct 2, 2026

View reviewed changes

olabusayoT force-pushed the daf-3065-build-cursor branch from c491292 to d5b6a70 Compare October 5, 2026 00:54

This was referenced Oct 6, 2026

Inline withRetryIfBlocking and the typed equality operators #1744

Merged

Add DAFFODIL_TDML_TUNABLES to run TDML suites under tunables #1745

Merged

olabusayoT force-pushed the daf-3065-build-cursor branch from d5b6a70 to cf5d3c0 Compare October 6, 2026 17:31

stevedlawrence reviewed Oct 6, 2026

View reviewed changes

olabusayoT force-pushed the daf-3065-build-cursor branch from f7db6da to 61f8d6d Compare October 9, 2026 17:06

jadams-tresys approved these changes Oct 9, 2026

View reviewed changes

olabusayoT force-pushed the daf-3065-build-cursor branch from 61f8d6d to 27e57a3 Compare October 9, 2026 21:23

olabusayoT added 2 commits October 13, 2026 14:59

fixup! Add a build ahead unparse path driven by an infoset builder tree

3e3a769

olabusayoT force-pushed the daf-3065-build-cursor branch from 27e57a3 to 3e3a769 Compare October 13, 2026 19:11

		finishBuildSide(infosetBuildState, rootUnparser)
		finishUnparseSide(unparseTreeState, rootUnparser)

Conversation

olabusayoT commented Sep 29, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

stevedlawrence left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stevedlawrence left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stevedlawrence left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

olabusayoT commented Sep 29, 2026 •

edited

Loading