Improve unparse suspension resolution and reduce clone overhead - #1717
Improve unparse suspension resolution and reduce clone overhead#1717olabusayoT wants to merge 1 commit into
Conversation
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
|
FYI using daffodil performance command |
|
|
||
| object MStackOfAnyRef { | ||
| def apply() = { | ||
| def apply(initialSize: Int = 32) = { |
There was a problem hiding this comment.
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.
| * initialSize lets a caller that knows its element count up front (e.g. | ||
| * cloning another MStack of known depth, see UState.cloneForSuspension) | ||
| * avoid the default 32-slot allocation when that's more than needed. | ||
| */ |
There was a problem hiding this comment.
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.
| case noLength: InfosetLengthUnknownException => | ||
| whereBlockedInfo.block(noLength.diElement, noLength.erd, 0, noLength) | ||
| // Register a targeted wake-up alongside the periodic-sweep blocking | ||
| // above: once this element's length becomes computable (see |
There was a problem hiding this comment.
I'm not sure what "periodic-sweep blocking above" is referencing.
| noLength match { | ||
| case _: InfosetContentLengthUnknownException => | ||
| whereBlockedInfo.registerLengthStateWaiter(noLength.diElement.contentLength) | ||
| case _: InfosetValueLengthUnknownException => | ||
| whereBlockedInfo.registerLengthStateWaiter(noLength.diElement.valueLength) | ||
| } |
There was a problem hiding this comment.
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.
| whereBlockedInfo.block(noLength.diElement, noLength.erd, 0, noLength) | ||
| // Register a targeted wake-up alongside the periodic-sweep blocking | ||
| // above: once this element's length becomes computable (see | ||
| // CaptureEndOf{Content,Value}LengthUnparser), this suspension is |
There was a problem hiding this comment.
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.
| * Must include suspensionsParked, or a suspension moving into that | ||
| * bucket would look like (incorrectly) resolved progress to a caller | ||
| * comparing this count before/after evalSuspensionsUnthrottled(). | ||
| */ |
There was a problem hiding this comment.
We should add a comment that this should only ever be used for debugging purposes. Combining these queues is a faiarly expensive operation that internally we should never use in a hot path.
| * discard-sink sweep before a real sweep (evalSuspensions) has run at | ||
| * least once. | ||
| */ | ||
| def parkedCount: Int = suspensionsParked.length |
There was a problem hiding this comment.
Never used, suggest we remove it
| * here unboundedly. | ||
| */ | ||
| def pendingCount: Int = | ||
| suspensionsYoung.length + suspensionsOld.length + suspensionsParked.length |
There was a problem hiding this comment.
Never used, suggest we remove
| * (e.g. padding/target-length SuspendableOperations), which must stay | ||
| * on the skip-and-requeue path so the real sweep still finds them. | ||
| * | ||
| * skipLengthStateWaiters is left false here: canResolveWithoutWriting |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
| * sweep's retries, and more completely (it also covers non-length | ||
| * forward references), so a second filter would be redundant. | ||
| */ | ||
| def evalBuildResolvableSuspensions(): Unit = |
There was a problem hiding this comment.
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?
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