Specify destroy() methods and behavior around context loss and error reporting

[inexorabletash]

Based on @mingmingtasd's work in the Chromium prototype implementation.

MLContext gains a destroy() method and a lost promise attribute. Calling destroy() releases resources for the context itself and any associated MLGraphs and MLTensors, and any outstanding build() requests for an associated MLGraphBuilder are aborted. The lost attribute then resolves, and any future use of the context fails. If the context is lost for other reasons, the same behavior occurs (releasing associated resources), and the lost promise provides an implementation-defined message explaining the reason.

MLGraph similarly gains a destroy() attribute. When destroyed - either by an explicit call, or because the associated context is lost - then any outstanding dispatch() requests using the graph are aborted.

This also modifies the omnipresent "has built" tests on MLGraphBuilder methods to be a "can built" test which also checks that the builder's context is not lost.

For #477

---

Preview | Diff

[inexorabletash]

Some points for discussion:

• This uses the internal slot [[lost]] as both (1) the promise value and (2) how to check if a context "is lost". Maybe an explicit boolean internal slot would be clearer, albeit more verbose?
• In the implementation, the "lost" state is equivalent to the context's mojo remote being connected. This is only checked explicitly when (1) creating an MLGraphBuilder and (2) creating an MLBuffer. This spec change does the former, but MLBuffer is not in the spec yet so that's not present. I did add an explicit check in compute(); I think that'll happen in the implementation indirectly because the dispatch would fail, but an explicit check seemed like a good idea?
• The MLContext section of the spec could be reorganized a bit. Separate PR?

@mingmingtasd and @reillyeon could you do an initial review?

[mingmingtasd]

Thanks! @inexorabletash @reillyeon
I am also working on a chromium CL to expose MLContext::destroy to make a context lost.

In the implementation, the "lost" state is equivalent to the context's mojo remote being connected. This is only checked explicitly when (1) creating an MLGraphBuilder and (2) creating an MLBuffer. This spec change does the former, but MLBuffer is not in the spec yet so that's not present.

I will check context lost for all of the synchronous and asynchronous actions depending on the context in my chromium CL.

[reillyeon]

Something this PR doesn't do is specify the behavior around rejecting in-flight asynchronous operations.

[inexorabletash]

Something this PR doesn't do is specify the behavior around rejecting in-flight asynchronous operations.

Thoughts on how to do that and to what level of detail?

• in compute(), is that handled by execute graph returning error, or do we need more steps?
• in build() it looks like failure other than "does not support a requested feature" isn't covered.

One generic approach for both of those would be to replace: "Queue an ML task with global to resolve promise with ..." with:

1. Queue an ML task with global to run these steps:
   1. If context is lost, then reject promise with an InvalidStateError.
   2. Resolve promise with ...

... which I think covers the script-observable behavior, but not that the async steps internally should fail.

[fdwr]

@RafaelCintron

[reillyeon]

There are two separate considerations: what happens to the promise returned by a method and what happens to an asynchronous operation itself. Operations like build(), compute() and dispatch() aren't abortable (or at least, the specification should not require that they are aborted, only that whether or not they are aborted is not visible to script). We can however be explicit that the promises returned by any methods on an object are synchronously rejected when the destroy() method is called or the context is lost. We could either specify this exactly the way the Chromium implementation works, by having each object hold a list of all promises and reject them, or we could add a note to the "in parallel" steps which says "when the context is lost or this is destroyed, reject promise and potentially abort these steps". The definition of "potentially abort" is the tricky one because it has to consider cases like destroying a single buffer that is part of a larger set of pending operations which should still be able to complete.

[huningxin]

@inexorabletash

• in compute(), is that handled by execute graph returning error, or do we need more steps?

I suppose the following step of execute graph could handle the device lost error:

Issue a compute request to graph.[[implementation]] given name and inputResources and wait for completion.

If that returns an error, then return an "OperationError" DOMException.

A question is if the error is device lost, should it run the "context-lost" steps? Like the Chromium prototype does in HandleComputationFailure(). Or we can just assume the "context-lost" steps are triggered by "When an MLContext context is no longer available to fulfill requests" asynchronously?

• in build() it looks like failure other than "does not support a requested feature" isn't covered.

Or we could just say "If that returns an error, then queue an ML task with global to reject promise with an 'OperationError' DOMException" similar to compute()?

One generic approach for both of those would be to replace: "Queue an ML task with global to resolve promise with ..." with:

1. Queue an ML task with global to run these steps:

   1. If context is lost, then reject promise with an InvalidStateError.
   2. Resolve promise with ...

The new steps may not run because these steps may already be aborted (by "abort these steps") if previous steps fail .

[bbernhar]

We can however be explicit that the promises returned by any methods on an object are synchronously rejected when the destroy() method is called or the context is lost.

+1. MLContext.destroy() steps could be spec to the following:

Script timeline:

1. Immediately reject all pending promises made off |this| context.
2. Issue steps for |this| context on the device/queue timeline.

Device/queue timeline:

1. Wait for async operations on the device to complete.
2. Then Lose |this| context.

Note: impl. is always free to abort pending async ops immediately (and release buffers).

[inexorabletash]

A question is if the error is device lost, should it run the "context-lost" steps? Like the Chromium prototype does in HandleComputationFailure(). Or we can just assume the "context-lost" steps are triggered by "When an MLContext context is no longer available to fulfill requests" asynchronously?

I think this would be script observable - i.e. what order do the lost and compute() promises settle. So we should spec it explicitly.

[mingmingtasd]

The definition of "potentially abort" is the tricky one because it has to consider cases like destroying a single buffer that is part of a larger set of pending operations which should still be able to complete.

@reillyeon @huningxin @bbernhar I think the DirectML backend here has considered this, MLBuffer can be destroyed prior to Compute() and Dispatch but its resource will be kept alive anyway until all the pending GPU work done. For example, if you destroy a MLBuffer used by Dispatch before the Dispatch is done, Dispatch can still complete but you can't continue to call ReadBuffer to read back the results. It seems OK and as expected?

[reillyeon]

I think we've settled on the idea that destroying an MLBuffer will only make readBuffer() fail. Pending compute tasks will still complete.

Destroying an MLContext (or context loss) is the equivalent of calling destroy() on all the builders, graphs and buffers created by the context. We haven't yet decided the semantics of destroying a graph but it will probably similarly allow pending compute tasks to complete.

[mingmingtasd]

I think we've settled on the idea that destroying an MLBuffer will only make readBuffer() fail. Pending compute tasks will still complete.

Destroying an MLContext (or context loss) is the equivalent of calling destroy() on all the builders, graphs and buffers created by the context. We haven't yet decided the semantics of destroying a graph but it will probably similarly allow pending compute tasks to complete.

I have submitted a CL to expose MLGraph::destroy : https://chromium-review.googlesource.com/c/chromium/src/+/5799069

[inexorabletash]

(not actually ready for review, but I wanted to force an updated preview to be generated)

[huningxin]

MLGraph may own device (e.g. GPU/NPU) resources. The destroy steps may need to release them on the context timeline. Worth adding a note? (Like MLTensor.destory() does)

[inexorabletash]

Added in 0fc4151 but it's a very generic note, it doesn't mention the timeline. Should it?

[reillyeon]

We can be a bit more explicit and queue a task on the context timeline to "mark resources owned by this graph as freeable".

[huningxin]

Is |tensor|.{{MLGraph/[[context]]}} a typo (an old issue)? Should it be |tensor|.{{MLTensor/[[context]]}} or just [=this=].{{MLContext/[[timeline]]}}? If it is a typo, the line 1096 also needs to be fixed. For example,

Suggested change

	1. Enqueue the following steps to |tensor|.{{MLGraph/[[context]]}}.{{MLContext/[[timeline]]}}, which [=/abort when=] [=this=] [=MLContext/is lost=]:
	1. Enqueue the following steps to [=this=].{{MLContext/[[timeline]]}}, which [=/abort when=] [=this=] [=MLContext/is lost=]:

[huningxin]

Another question is whether it should check context lost earlier? Like createTensor steps do, e.g.

1. If [=this=] [=MLContext/is lost=], then return [=a new promise=] [=rejected=] with an "{{InvalidStateError}}" {{DOMException}}.

[inexorabletash]

Given that the context loss will destroy every associated tensor, I don't think that is needed. I'm not opposed to adding it explicitly though.

[inexorabletash]

For the "type confusion" I added a lint tool test and caught several more places. It's not perfect, but it is progress!

[reillyeon]

We can be a bit more explicit and queue a task on the context timeline to "mark resources owned by this graph as freeable".

[huningxin]

LGTM!

[fdwr]

Thanks. I scanned it, and it looks fine (not as close scrutiny given Reilly and Ningxin already looked). I'll delay merging a bit though because Rafael wanted to look at it tomorrow. ⏳

[RafaelCintron]

LGTM. @inexorabletash , thank you very much for putting this together.

[mwyrzykowski]

Also looks good from WebKit's side @inexorabletash - informal approval from discussion in today's call