-
Notifications
You must be signed in to change notification settings - Fork 46
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Don't transfer input ArrayBuffers #566
Comments
Agreed.
This is true. Should we consider copying the content of input (and constant) |
Given the constraints on implementations based on the underlying platform APIs I think it's reasonable to define constant and input handling such that In theory Chromium's XNNPACK backend could avoid copies but I'm also planning to delete it in favor of a more flexible TFLite backend. The zero-copy option only works for CPU backends anyways (without |
Renamed this issue given my current position on how to resolve the inconsistency between handling of constant and input |
- Adds validation that passed outputs are neither inputs nor constants, matching the Chromium implementation. - Traverses the graph, starting with outputs, to visit all connected operands. - Previously the outputs were iterated over to process inputs and constants, which didn't make any sense. Inputs are hooked up to [[inputsDescriptors]]. Nothing is specifically mentioned about constants, but an issue about transferring is referenced. (webmachinelearning#566) - The impact of MLGraphBuilder re-use (webmachinelearning#567) is called out, since it could allow for removing the graph traversal. - Populates graph's [[outputDescriptors]], which was previously just missing. (webmachinelearning#448) - Makes most of the validation behavior of build() happen synchronously rather than "in parallel". A promise is still returned, of course. - Converting the graph into an implementation-defined format is called out, which remains in "in parallel" steps. Fixes webmachinelearning#448, fixes webmachinelearning#457, fixes webmachinelearning#552.
* Content: Define build() steps more rigorously - Adds validation that passed outputs are neither inputs nor constants, matching the Chromium implementation. - Traverses the graph, starting with outputs, to visit all connected operands. - Previously the outputs were iterated over to process inputs and constants, which didn't make any sense. Inputs are hooked up to [[inputsDescriptors]]. Nothing is specifically mentioned about constants, but an issue about transferring is referenced. (#566) - The impact of MLGraphBuilder re-use (#567) is called out, since it could allow for removing the graph traversal. - Populates graph's [[outputDescriptors]], which was previously just missing. (#448) - Makes most of the validation behavior of build() happen synchronously rather than "in parallel". A promise is still returned, of course. - Converting the graph into an implementation-defined format is called out, which remains in "in parallel" steps. Fixes #448, fixes #457, fixes #552. * Update index.bs Co-authored-by: Reilly Grant <reillyeon@users.noreply.github.com> * Build collection of input operands, and simplify steps * Populate operand and operator sets too --------- Co-authored-by: Reilly Grant <reillyeon@users.noreply.github.com>
#700 naively addresses this issue by copying (rather than transferring) inputs. This is a bit awkward, though:
Thoughts on these changes? dictionary MLComputeResult {
- MLNamedArrayBufferViews inputs;
MLNamedArrayBufferViews outputs;
};
-Promise<MLComputeResult> compute(MLGraph graph, MLNamedArrayBufferViews inputs, MLNamedArrayBufferViews outputs);
+Promise<MLComputeResult> compute(MLGraph graph, MLNamedArrayBufferViews inputs); Note that we'd like to deprecate |
(moved the comment here from the PR) |
The closest I've found (thanks to Webdex) are these annotated invocations:
The one in Encoding is probably the closest to what you're looking for:
I think the equivalent here would be to add an informative note to the call sites (in #700, "Let copiedInputs be the result of copying MLNamedArrayBufferViews inputs ...") describing why a copy is made. I think this is helpful for reviewers, future editors, and implementers to understand why it is specified this way. This could be written as just |
Thanks for the input, @inexorabletash. Adding a
Any objections to expanding the scope of this issue to the above diff? i.e. Don't transfer input ArrayBuffers, and return output ArrayBuffers rather than passing them to |
Accepting a typed array to write the result into (or "BYOB") is a valuable pattern for reducing memory allocation overhead (and thus garbage collection overhead). I don't think it's worth changing the interface here but we should consider providing a BYOB version of |
I've prototyped a BYOB version of |
Right now the input and output
ArrayBuffer
s passed toMLContext
'scompute()
method are transferred so that their contents cannot be modified by script during the computation. It seems appropriate that anArrayBuffer
passed toMLGraphBuilder
'sconstant()
method should be treated the same way, though it would be better if the transfer were delayed untilbuild()
is called so that multipleArrayBufferView
s using the same backingArrayBuffer
can be passed as constants.build()
would construct a list of unique backingArrayBuffer
s to transfer rather than the naive approach of simply iterating over theArrayBufferView
s themselves.I'll note that transferring
ArrayBuffer
parameters makes an API harder to call from WebAssembly, but I think that any argument for why compute inputs and outputs are transferred should also apply to constants.The text was updated successfully, but these errors were encountered: