Improve unparse suspension resolution and reduce clone overhead

[olabusayoT]

Suspensions blocked on a not-yet-known DFDL length (dfdl:valueLength, dfdl:contentLength) now register directly on the referenced element's LengthState and get retried the moment that length becomes known, instead of waiting for SuspensionTracker's periodic sweep (LengthState.notifyWaiters, InfosetImpl.scala; registration in DPath.scala's InfosetLengthUnknownException handling).

SuspensionTracker gains a dedicated pendingCount, a separate suspensionsParked queue for suspensions with a registered targeted wake-up (removed from the periodic rotation entirely, pruned once resolved so a suspension that resolves out-of-band doesn't sit retained for the rest of the document), and
evalBuildResolvableSuspensions, a sweep variant for callers that can't write real bytes yet (a discard-sink traversal), gated by Suspension.canResolveWithoutWriting.

Suspension.suspendWithoutAttempting registers and tracks a suspension without ever calling doTask, for callers that already know, from static information doTask itself can't see, that the first attempt is certain to block; used by ElementOVCSpecifiedLengthUnparser when its expression can never resolve without a real written DOS bit position.

cloneForSuspension (UState.scala) sizes its escapeSchemeEVCache/ delimiterStack MStack clones to the source's actual depth instead of MStack's default 32-slot allocation.

DAFFODIL-3065

[olabusayoT]

FYI using daffodil performance command

+---------+-----+------------+----------+----------+-----------+---------+----------+------------+-----------+-------------+                                                                                                      
|       N |   T |  main tput |   A tput |   tput Δ |  main lat |   A lat |    lat Δ |  main susp |    A susp |  susp match |                                                                                                      
+---------+-----+------------+----------+----------+-----------+---------+----------+------------+-----------+-------------+                                                                                                      
|       1 |   1 |       1.45 |     1.46 |    +0.3% |    672.44 |  669.81 |    -0.4% |       4000 |      4000 |         yes |                                                                                                      
|       1 |   4 |       1.44 |     1.48 |    +3.0% |    679.15 |  658.51 |    -3.0% |       4000 |      4000 |         yes |                                                                                                      
|       1 |   8 |       1.43 |     1.49 |    +4.0% |    683.19 |  656.02 |    -4.0% |       4000 |      4000 |         yes |                                                                                                      
|    2500 |   1 |      55.16 |    60.04 |    +8.8% |     18.13 |   16.65 |    -8.2% |   10000000 |  10000000 |         yes |                                                                                                      
|    2500 |   4 |     131.90 |   145.82 |   +10.6% |     30.28 |   27.39 |    -9.6% |   10000000 |  10000000 |         yes |                                                                                                      
|    2500 |   8 |     131.73 |   139.70 |    +6.0% |     60.62 |   57.15 |    -5.7% |   10000000 |  10000000 |         yes |                                                                                                      
|    5000 |   1 |      61.54 |    67.66 |    +9.9% |     16.25 |   14.79 |    -9.0% |   20000000 |  20000000 |         yes |                                                                                                      
|    5000 |   4 |     152.76 |   181.06 |   +18.5% |     26.16 |   22.10 |   -15.5% |   20000000 |  20000000 |         yes |                                                                                                      
|    5000 |   8 |     153.04 |   168.74 |   +10.3% |     52.24 |   47.36 |    -9.3% |   20000000 |  20000000 |         yes |                                                                                                      
|   10000 |   4 |     163.49 |   182.48 |   +11.6% |     24.45 |   21.91 |   -10.4% |   40000000 |  40000000 |         yes |                                                                                                      
|   10000 |   8 |     168.06 |   178.36 |    +6.1% |     47.57 |   44.83 |    -5.8% |   40000000 |  40000000 |         yes |                                                                                                      
+---------+-----+------------+----------+----------+-----------+---------+----------+------------+-----------+-------------+

[stevedlawrence]

Should the other MStacks, e.g. MStackOfBoolean, MStackofInt have this initialize size field in the apply method too. I guess we don't currently use them, but they should be available.

[stevedlawrence]

I believe the init function is not a function that users should call. So this user-related documentation probably doesn't belong here. I would suggest whatever documentation we do have for initialSize wants to be on the actual user visible apply methods. I'd also suggest not provided examples referencing other code--that code could change and then this documentation is wrong and potentially confusing. If we do want to provide examples, we can include example usage in this comment. That said, initialSize is pretty self explanatory, so I'm not sure an example or even documentation provides much value.

Also, I wonder if we can get rid of the init function entirely, e.g. just do this in the constructor:

private var table: Array[T] = arrayAllocator(initialSize)

Currently all the MStack* constructors are priate so you have to use the object apply method, and I think all the objects immediately call init after allocating an MStack instance. So the init function doesn't seem to really do anything special. Maybe we should just get rid of it if possible and simplify the code.

[stevedlawrence]

I'm not sure what "periodic-sweep blocking above" is referencing.

[stevedlawrence]

I think if we make lengthState a val in InfosetLengthUnknownException we can avoid the match/case and simplify this to:

whereBlockedInfo.registerLengthStateWaiter(noLength.lengthState)

I don't think there's any harm in making lengthState accessible and it simplifies our code.

[stevedlawrence]

I think we might have to be careful about where we call notifyWaiters--I don't think CaptureEndOf*LengthUnparser is the only place where the length of an element can be finally resolved. In fact, I think it's not uncommon for those unparsers to not be able to actually capture the actual length and instead just capture relative bit positions of where the DOS started, which often isn't enough for

I'm wondering if instead of notifying when an unparser ends, we should instead notify when something calls setRel/AbsBitPosition on the LengthState, since 1. that is the LengthState that supension is waiting for and 2. the LengthState knows when it has been calculated.

The one thing I'm not sure about is if the length gets recalculated during normal buffer resolution or when something (i.e. the suspension) asks for the length. If it'sonly recalculated when a suspension asks for it, then we're in a catch-22--we won't notify the suspension until the length is recalculated, butwe won't recalculate the length until the suspension is run.

I think if we notify when things like setAbs/Rel are called, and maybe some ofther functions, it might be sufficient to ensure the suspensions are always triggered correctly.

[stevedlawrence]

Never used, suggest we remove it

[stevedlawrence]

Never used, suggest we remove

[stevedlawrence]

It feels like skipLengthStateWaiters can go away if we move to a deisgn where we never evaluate LengthState waiters until they are notified, and when they are notified we move them to young since they are likely to succeed. With that design, we always skip length state waiters until something moves them to the young queue.

[olabusayoT]

Unfortunately some test in our current test rig break with the proposed "lengthstate suspensions never evaluate until notified" plan, because of DOS Splitting/merging, so in those situations, the registered wakeup never gets fired. Here's is a more details explanation from claude

Here's what's actually going on in TestOutputValueCalc1's OutputValueCalc_01:

Trace (OutputValueCalc_01: x needs valueLength(y), y needs valueLength(z)):

first-attempt SuspendableExpression(x, valueLength(y)) → isDone=false (y unwritten) 
first-attempt SimpleTypeRetryUnparser(x) → isDone=false (x has no value) 
first-attempt SuspendableExpression(y, valueLength(z)) → isDone=false (z unwritten) 
first-attempt SimpleTypeRetryUnparser(y) → isDone=false (y has no value) 

retry SuspendableExpression(x) isWaitingOnLengthState=false 
registerWaiter valueLen(y) <- SuspendableExpression(x) ← x registers its ONE targeted wake-up, on y's length 
retry-result SuspendableExpression(x) → isDone=false 

retry SimpleTypeRetryUnparser(x) → isDone=false 
retry SuspendableExpression(y) → isDone=true (z is a plain string, already known) 
retry SimpleTypeRetryUnparser(y) → isDone=true (y's bytes now written) 
retry SimpleTypeRetryUnparser(x) → isDone=false (still no value for x) 
retry SimpleTypeRetryUnparser(x) → isDone=false 

retry SuspendableExpression(x) isWaitingOnLengthState=true ← STILL registered, no notify ever fired 
retry-result SuspendableExpression(x) → isDone=true ← succeeds anyway, via a plain re-attempt 

The point: x registers exactly once, against y's ValueLengthState. y's length never triggers that registered wake-up as y's length only becomes computable as a side effect of y's own SimpleTypeRetryUnparser succeeding (a completely different suspension object, blocked on y's value, not on any length at all) plus the surrounding DOS-splitting machinery converging. Grepping the run for the wake-up call (moveFromParkedToYoung, the only path a registered notify can take) and for the notify-check itself found zero hits. x succeeds purely because something keeps giving it a plain, unconditional re-attempt, not because anything ever told it "your wait is over."

So "never evaluate a suspension until it's notified" is unsound as a general design: x's only completion path here is a blind retry with a stale, never-fired registration still sitting on it. A purely event-driven model has no event to catch this, which is exactly why the periodic retry has to stay.

[stevedlawrence]

Ah right, eventually the state of DOSs change, which allows LengthState to be calculated. But the LengthState never knows that the DOS state changed. Maybe an option is to add a member to DOSs to track the LengthState's that have suspensions that depend on that DOS? When the DOS state changes it could notify the LengthStates that they could be resolvable, and then the LengthStates could unpark the suspension?

I also wonder if this approach can be generalized--it sounds like someting we could potentially use for other suspensions. Essentially, we have things that keep track of certain Suspensions because they are waiting for state to change to the point whre those suspensions are likely to be resolvable and then things that notify those suspensions that the calculation could succeed. For example, maybe something like this:

class SuspensionWaiter {

	// list of suspensions that are parked until something notifies this waiter
	val suspensions = mutable.Set[Suspension].empty

	// list of classes that maintain some state that when changed could notify this waiter
        // that their suspensions might now be resolvable
	val notifiers = mutable.Set[SuspensionWaiterNotifier].empty

	def addSuspension(s: Suspension) = {
		suspensionTracker.park(s)
		suspensions.addOne(s)
	}

	def notify(): Unit = {
		suspensions.foreach { s => suspensionTracker.moveFromParkedToYoung(s) }
		suspensions.clear()
		notifiers.foreach {
			_.waiters.remove(this)
		}
	}
}

class SuspensionWaiterNotifier {
	val waiters = mutable.Set[SuspensionWaiter].empty

	def addWaiter(w: SuspensionWaiter) = waiters.addOne(w)

	def notify(): Unit = {
		waiters.foreach(_.notify)
	}
}

And then in LengthState, this might look something like

class LengthState {

	// already existing members
	var maybeStartDataOutputStream: Maybe[DataOutputStream] = Nope
	var maybeEndDataOutputStream: Maybe[DataOutputStream] = Nope

	// new waiter val
	val suspensionWaiter = new SuspensionWaiter {
		override def addSuspension(s: Suspension) {
			super.addSuspension(s)
			maybeStartDataOutputStream.foreach { dos => dos.suspensionWaiterNotifier.addWaiter(this) }
			maybeEndDataOutputStream.foreach { dos => dos.suspensionWaiterNotifier.addWaiter(this) }

		}
	}
}

And when a suspension is blocked on a LengthState, it would do

lengthState.suspensionWaiter.addSuspension(suspension)

When this is called, the suspension gets parked, and the DOSs are told to notify the waiter if something changes.

And then the DataOutputStreams has something like

class DataOutputStreamImplMixin {

	val suspensionWaiterNotifierLengthState = new SuspensionWaiterNotifier()

	// ... when DOS state changes where a length state might be resolvable
	suspensionWaiterNotifierLengthState.notify()
}

That will notify each of the waiters that depend on its state (if any). The waiters will then move their suspensions from parked to young to be evaluated some point soon. And then they'll tell the notifiers to no longer notify this length state waiter since it no longer is waiting on suspensions.

Also note that the LengthState class could also call suspensionWaiter.notify(), for example if setAbsBitPosition is changed--a waiter does not have to be notified by the SuspensionWaiterNotifiers. And some suspensionWaiter might not even have notifiers. Those are just a helpful class to use when other classes maintain state that the waiter classes depend on.

Also, note that the Notifier could override the notify() function to actually examine state and determine if it really should unpark the suspension and clean up the state. The notify() function doesn't necessarily require unparking. But the default, and likely most common implementation, could be to just unpark the suspension, let things run, and if they block again then they just register with suspensionWaiter/Notifiers and repeat the process.

"never evaluate a suspension until it's notified" is unsound as a general design:

Note that we should still unpark all parked suspensions when tracker.requireFinal is called, which should allow them to be resolved. So even if something is never notified (which is probably a bug and we should probably warn) we could still have a backup to ensure things at least succeed.

---

Also, there are likely other designs. For example, maybe the SuspensionWaiter becomes part of a the Suspension class, and parked Suspension's just keep track of a list of SuspensionNotifiers. Then the both the DOS and LengthState classes would have SuspensionNotifiers which keep track of the Suspensions that depend on them. One downside to this approach is it doesn't allow LenghtStates to overrule a notification from a DOS if it wants, since ultimately the LengthState knows if the change to the DOS state makes a difference to the LengthState or not. If we moved the waiters into Suspensions, a DOS would unpark the and there's nothing the LengthState waiters could do about it. But maybe we don't need that level of granularity for this. It does feel a bit simpler to not really have a separate concept of SuspensionWaiters, and instead there are only things that can notify a Suspension that it could be unparked.

[stevedlawrence]

This is never used, and it's purpose isn't entirely clear, presumably for a future change. Can this and things related to filterToBuildResolvable be moved to a separate PR so we can review all the related stuff in one PR?

[stevedlawrence]

Looks nice, I think we need additional documentation since it's still not totally clear to me how the park stuff interacts with the young/old queues, and when things are supposed to moved from one to the other. There's also maybe some room for some simplfication, which feels important since I think this is all just going to get even more complex as we deal with suspensions.

[stevedlawrence]

I think this needs a new name to make it clear this is about a DOS finishing. I think a number of things in Daffodil have a concept of being finishes/finalized, so that trait and allbacks probably want to differentiate that to make it clear what is actually being finished. You could maybe getaway with notifyFinished as the call back name if it also accepts the DOS as a parameter so the callback knows which one was finished if it cares.

[stevedlawrence]

I'm wondering if this wants to be a var immutable Set?

My thinking is that in most cases there will probably zero or just a very small number of listeners, and immutable Sets are optimized in those cases, with special EmptySet, Set1, Set2, Set3, and Set4 classes. And beyond that it is still fairly efficient to avoid copies when adding new elements.

This would clean up the Maybe isEmpty/get stuff and also remove overhead related to hashsets (e.g. hash calculations, array allocations, bucket allocations, etc). I'm not sure it will make much of a difference in practice since these are almost always empty and I imagine a Nope and a Set.Empty are basically the same.

[stevedlawrence]

One drawback with this notifying only on is finished is suspensions don't necessarily need a DOS to be finished to be able to resolve. In some cases they might just need the starting absolute bit position to allow length to be calculatable. isFinish will work, but it might delay the suspension for much longer.

This is one reason where the suggested approach about making this more specific to suspensions has advantages. This could notify the suspension waiters when maybeAbStartingBitPos0b gets set rather than waiting for it to be finished. The waiter can then examine the state and act accordingly. It's also more generic and could be used in other suspension optimizations that wait on different state. This approach only works for DOSs, we'll need a new mechanism if we want some other kind of listern (e.g. element inf InfosetImpl becomes final).

[stevedlawrence]

Can we just do maybeFinishedListeners.get.foreach(_.notifyFinished()) and avoid the array allocation?

[stevedlawrence]

We can just set maybeFinishedListerns = Nope and let the hash set be garbage collected, once this is finihsed we'll never need the hashSet again,so we don't really care what its state is. And I imagine the clear() function zero's out the backing array which is going to be a bit slower.

[stevedlawrence]

Suggest we just make these public

[stevedlawrence]

This probably just wants to be

suspensions.foreach { ... }
suspensions.clear()

No need to copy to an array first

[stevedlawrence]

If you make this a @transient lazy val then you shouldn't need the readObject. On deserialization a lazy val just becomes unset and so the first time it's referenced a new one will be created.

[stevedlawrence]

I've avoid mentioning how this differs from the length. Just describe how this specific implemenation works. If we ever change how length works we don't need to update this unrelated comment.

[stevedlawrence]

I'm not sure what this comment is trying to say. Sounds like a fancy way to saw what the original comment said. we need to run the expression, it might need to succeed be something it's known yet, which is just standard suspension behavior.

[olabusayoT]

Sorry for the delayed results. WE needed some performance updatesafter profiling

  +----------+---+---------------+---------+---------------------------+                                                                                                                                                            
  | workload | t | branch vs main  | Welch t | significant?              |                                                                                                                                                            
  +----------+---+---------------+---------+---------------------------+                                                                                                                                                            
  | sparse   | 1 | -19.52%       | -19.07  | yes, extremely            |                                                                                                                                                            
  | sparse   | 4 |  -5.65%       |  -3.22  | yes                       |                                                                                                                                                            
  | dense    | 1 |  +1.27%       |   0.80  | no - essentially even     |                                                                                                                                                            
  | dense    | 4 |  -1.05%       |  -0.26  | no - essentially even     |                                                                                                                                                            
  +----------+---+---------------+---------+---------------------------+

  +--------+----+----------------------------+-------------+-------------+---------+---------+                                                                                                                                      
  | workload | t | metric                    |    main     |    brance     |  pct    | Welch t |                                                                                                                                      
  +--------+----+----------------------------+-------------+-------------+---------+---------+                                                                                                                                      
  | sparse | 1  | throughput (files/sec)     |     8.940   |    11.100   | +24.16% |  20.28  |                                                                                                                                      
  | sparse | 1  | latency (ms)               |   111.902   |    90.035   | -19.54% | -19.08  |                                                                                                                                      
  | sparse | 4  | throughput (files/sec)     |    19.525   |    19.964   |  +2.25% |   0.53  |                                                                                                                                      
  | sparse | 4  | latency (ms)               |   203.616   |   192.270   |  -5.57% |  -3.13  |                                                                                                                                      
  | dense  | 1  | throughput (files/sec)     |     0.491   |     0.485   |  -1.15% |  -0.74  |                                                                                                                                      
  | dense  | 1  | latency (ms)               |  2039.470   |  2065.425   |  +1.27% |   0.80  |                                                                                                                                      
  | dense  | 4  | throughput (files/sec)     |     1.060   |     1.039   |  -1.95% |  -0.44  |                                                                                                                                      
  | dense  | 4  | latency (ms)               |  3438.972   |  3550.129   |  +3.23% |   0.53  |                                                                                                                                      
  +--------+----+----------------------------+-------------+-------------+---------+---------+

[jadams-tresys]

A few cleanup suggestions

Have you verified any of the performance gains on any of our actual schemas? I know I ran our usual performance tests on some of these changes but wanted to check if any other performance testing had been done outside of fabricated test schemas.

[jadams-tresys]

Does it make sense to have this comment bet 20+ lines away from where the call actually is? I'd probably just remove this comment.

[jadams-tresys]

Is there any gain from having this overflow logic as opposed to just having the mutable.ArrayBuffer and always calling .tail to get the last entry? We can still initialize it with just 1 entry so I wouldn't think there would be a significant allocation overhead and it would make a lot of this much simpler.

[olabusayoT]

Yes, it's a performance gain that keeps us from allocating an array buffer on every single suspension waiter (which are mostly zero registrants), as opposed to only when they are actual registrants.

This came out of a profiling session for allocation hotspots

[jadams-tresys]

I feel like the 2 tests in this file could just be TDML tests or should at least make use of the TestUtils code instead of recreating a bunch of existing functionality

[olabusayoT]

Good catch! Will do

[stevedlawrence]

+1, it feels like this still adds lots of complication that is hard to follow. But I don't see anything clearly wrong and tests seem to pass, and it does show performance gains. I suspect some day we might want to revisit this and see if there is a simpler design that gives similar results, but seems a resonable first step towards improving suspension performance

[stevedlawrence]

Feels like this wants to be a val, this should never change during normal usage. This is purely for developers to determine if certain stacks should have a different default size. I don't thin it's unreasonable to require a recompile to do that kind of testing.

[stevedlawrence]

I think this logic to track middle doses is new from previous changes. Did you notice if that made any impact to performance. It adds complexity and now I'm wondering if that added complexity is wroth it. Maybe only the start and end DOSes are all that really matters in most cases and we can just wait for those to be finished.

[stevedlawrence]

Suggest we remove these IfAllocated suffix. It seems reasonble to make this a no-op if there's no suspension waiter. I don't think callers would care. Just add documentation to the functions

[stevedlawrence]

Might want to add a TODO to this comment?

[olabusayoT]

This comment predates this branch, all we did want wrap it in a try block.. I can add a todo statement tho

[stevedlawrence]

Feels odd for the Suspension class have something specific about DOSes. I thought the goal of the SuspensionWaiter logic was so that outside things can track whatever state they care about (whehter it be DOSes, Variables, Nodes, etc.), and when their state changes they notify suspension that they are/might be ready to be reevaluated.

[olabusayoT]

There are two different registry classes:

• DataOutputStreamListenerRegistry[L] (DataOutputStream.scala:68-91) lives on the DOS side. Each DirectOrBufferedDataOutputStream owns one, holding the set of listeners waiting on that one DOS's fact (Set[L]). One DOS can have many listeners. This answers "who do I tell when I change?"
• DosRegistrations (Suspension.scala:309-328) lives on the Suspension side. It holds the set of DOSes this one suspension has registered itself with (Set[DataOutputStream]). One suspension to many DOSes (ex: a length calc suspensions can be watching both its start and end DOS at once). This answers "what do I need to unregister from when I'm done?"

[olabusayoT]

Note this above is specific to DOSes, variable/nodes use suspension waiters maybeRegisteredWaiters path

[stevedlawrence]

How complex would it be for suspensions to use something similar for DOSes? It feels like Suspensions don't want to have to know details about things, it's the SuspensionWaiters that want to know those details.

[olabusayoT]

No performance gains for the added complication