Transfer the input and output views for asynchronous execution #323

huningxin · 2023-01-06T04:01:58Z

@wchao1115 @anssiko @wacky6 @RafaelCintron , please take a look. Thanks!

anssiko · 2023-01-09T13:44:48Z

@domenic, as the author of the ReadableStreamBYOBReader design, you may want to look at this PR inspired by your work.

wchao1115

Sorry for delay. I was sick the last few days.

wchao1115 · 2023-01-10T04:27:46Z

index.bs


 <script type=idl>
+dictionary ComputeResult {


To be naming-consistent with the rest of the API, should we also prefix this type with ML e.g. MLComputeResult?

Yes, we should. Fixed in the latest commit.

wchao1115 · 2023-01-10T04:33:53Z

index.bs

+const result = await context.compute(graph, inputs, outputs);
+// The computed result of [[1, 1], [1, 1]] is in the buffer associated with
+// the output operand.
+console.log('Output value: ' + result.C);


Should this be result.outputs.C?

Sorry, my typo. Fixed in the latest commit.

wchao1115 · 2023-01-10T04:34:56Z

index.bs


-console.log('Output value: ' + outputBuffer);
+console.log('Output value: ' + result.output);


result.ouputs.output?

Thanks for catching this bug! fixed in the latest commit.

huningxin · 2023-01-10T06:10:00Z

@wchao1115

Sorry for delay. I was sick the last few days.

No worries. Hope you are feeling better now. Thanks for your review and comments. I uploaded a new commit to address your comments. Please take another look.

domenic

Some minor suggestions on how to better use Web IDL wrapper algorithms, which are in general preferable to directly using JS spec primitives.

However, it appears we haven't added sufficient wrapper algorithms to Web IDL to replace the Construct() step. So directly using JS spec primitives there is fine, if a bit unfortunate.

There is a more dramatic change I would suggest, if possible. It would be better if your spec algorithms operated on byte sequences instead of ArrayBufferViews. That is, you would:

Accept MLNamedArrayBufferViewss.
Transfer the underlying buffers.
Get a copy of the bytes of that buffer. (In a real implementation, this would not make a copy, but in the spec, it's a convenient way to extract the byte sequences and get away from the IDL types. Since the copy is unobservable, this is fine.)
Modify those byte sequences.
Eventually, create ArrayBufferViews from those byte sequences. (Again, the algorithm here is a copy, but an unobservable one that implementations will thus elide.)
Then stuff those into new MLNamedArrayBufferViewss to resolve your promise with.

However, I'm not sure how well this would work for your case exactly. It seems like you care about the view types and such, and tracking that separately might be a pain.

Note that pretty much all of this advice is about how to do things better than the Streams Standard currently does them. Streams was written before we added appropriate helpers to Web IDL, and we haven't taken the time to update Streams yet. That also means people haven't really battle-tested the Web IDL helpers, so they might not work perfectly :). But the above is how I think it should work...

domenic · 2023-01-10T06:11:13Z

index.bs

+To <dfn for="MLNamedArrayBufferViews">transfer</dfn> an {{MLNamedArrayBufferViews}} |views|:
+1. Let |transferredViews| be a new {{MLNamedArrayBufferViews}}.
+1. For each |key| -> |value| of |views|:
+    1. Let |transferredBuffer| be the result of [=transfer array buffer|transferring=] {{ArrayBuffer}} |value|.\[[ViewedArrayBuffer]].


Use https://webidl.spec.whatwg.org/#buffersource-underlying-buffer instead of consulting [[ViewedArrayBuffer]]

Thanks for the pointer. Fixed by the latest commit.

domenic · 2023-01-10T06:11:47Z

index.bs

 urlPrefix: https://webidl.spec.whatwg.org/; spec: WEBIDL
    type: interface
        text: Promise; url: idl-promise
        text: nullable; url: idl-nullable-type
+    type: dfn
+        text: transfer array buffer; url: arraybuffer-transfer


This should not be necessary if you use [=ArrayBuffer/transfer=] syntax.

Sounds good. Fixed it in the latest commit.

domenic · 2023-01-10T06:13:26Z

index.bs

+1. For each |key| -> |value| of |views|:
+    1. Let |transferredBuffer| be the result of [=transfer array buffer|transferring=] {{ArrayBuffer}} |value|.\[[ViewedArrayBuffer]].
+    1. Let |constructor| be the appropriate [=view constructor=] for the type of {{ArrayBufferView}} |value|.
+    1. Let |elementsNumber| be the result of |value|.\[[ByteLength]] ÷ [=element size=] of |value|.


Use https://webidl.spec.whatwg.org/#buffersource-byte-length instead of accessing [[ByteLength]] directly.

Thanks for the pointer. Fixed it in the latest commit.

anssiko · 2023-01-10T07:29:30Z

@domenic much thanks for your detailed advice on behalf of the whole WG!

huningxin · 2023-01-11T13:29:12Z

@domenic , I'd like to echo @anssiko and thank you for the detailed feedback! It is very helpful.

Some minor suggestions on how to better use Web IDL wrapper algorithms, which are in general preferable to directly using JS spec primitives.

I pushed a new commit that uses the Web IDL wrapper algorithms as you suggested.

There is a more dramatic change I would suggest, if possible. It would be better if your spec algorithms operated on byte sequences instead of ArrayBufferViews.

Thanks for this proposal! I feel the byte sequences algorithm would help improve the graph execution steps that @zolkis is working on by #319.

Because this PR mainly focuses on fixing #318 by transferring the input/output buffers, we probably can file a separate issue to track this proposal and fix it by a follow-up PR or Zoltan's PR #319.

WDYT?

zolkis · 2023-01-11T13:39:45Z

Because this PR mainly focuses on fixing #318 by transferring the input/output buffers, we probably can file a separate issue to track this proposal and fix it by a follow-up PR or Zoltan's PR #319.

From my part I am fine with that. Please finish what you wanted in this PR and I will rework (not only rebase) my PR based on that.

anssiko

@huningxin thanks! Approved with one suggestion and with an assumption @zolkis' upcoming update address the byte sequences algorithm bits.

index.bs

anssiko · 2023-01-13T11:47:53Z

@wchao1115 PTAL

As discussed yesterday, we'd like to get this PR landed soon so @zolkis can rework his PR #319 on top of this. Thanks!

anssiko · 2023-01-18T09:08:52Z

Thank you everyone! This PR has received adequate review and we're now ready to merge to unblock related work.

I'm especially pleased to see how the cross-group collaboration helped improve the design.

SHA: 019a1f6 Reason: push, by anssiko Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

Signed-off-by: Zoltan Kis <zoltan.kis@intel.com>

To align with latest spec: webmachinelearning/webnn#323 This fixed: webmachinelearning#158

To align with latest spec: webmachinelearning/webnn#323 This fixed: #158

…rning#323 Squashed from the following commits: Add link to asserts Clarify the graph execution steps Address review comments for graph execution Factor out buffer validation vs descriptor Remove note about execution Signed-off-by: Zoltan Kis <zoltan.kis@intel.com>

* Rework the sync and async execution algorithms based on #323 Squashed from the following commits: Add link to asserts Clarify the graph execution steps Address review comments for graph execution Factor out buffer validation vs descriptor Remove note about execution Remove extra copy of the 'validate buffer with descriptor' steps Signed-off-by: Zoltan Kis <zoltan.kis@intel.com>

Transfer the input and output views for asynchronous compute

aa90e8a

fix webmachinelearning#318

huningxin requested review from anssiko, wchao1115 and RafaelCintron January 6, 2023 04:01

anssiko mentioned this pull request Jan 9, 2023

WiP: Improve graph execution steps #319

Closed

wchao1115 reviewed Jan 10, 2023

View reviewed changes

Address Chai's comments

d332194

domenic reviewed Jan 10, 2023

View reviewed changes

Address Domenic's comments

a750ea4

anssiko approved these changes Jan 12, 2023

View reviewed changes

index.bs Outdated Show resolved Hide resolved

Wrap the informative text into a note block

715795c

wchao1115 approved these changes Jan 18, 2023

View reviewed changes

anssiko merged commit 019a1f6 into webmachinelearning:main Jan 18, 2023

github-actions bot added a commit that referenced this pull request Jan 18, 2023

Merge pull request #323 from huningxin/buffer_transfer

6a4ed76

SHA: 019a1f6 Reason: push, by anssiko Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>

zolkis added a commit to zolkis/webnn that referenced this pull request Jan 18, 2023

Rework the sync async algorithms based on webmachinelearning#323

7bd5cc2

Signed-off-by: Zoltan Kis <zoltan.kis@intel.com>

This was referenced Feb 1, 2023

Transfer the input and output views for asynchronous execution webmachinelearning/webnn-polyfill#200

Closed

[webnn] Updated result of asynchronous execution. web-platform-tests/wpt#38343

Merged

Honry mentioned this pull request Feb 6, 2023

Return async compute() outputs webmachinelearning/webnn-samples#158

Closed

Honry added a commit to Honry/webnn-samples that referenced this pull request Feb 15, 2023

Return asyn compute() outputs

ff84cb9

To align with latest spec: webmachinelearning/webnn#323 This fixed: webmachinelearning#158

Honry mentioned this pull request Feb 15, 2023

Return async compute() outputs webmachinelearning/webnn-samples#159

Merged

Honry added a commit to webmachinelearning/webnn-samples that referenced this pull request Feb 15, 2023

Return asyn compute() outputs (#159)

087318b

To align with latest spec: webmachinelearning/webnn#323 This fixed: #158

anssiko mentioned this pull request Jun 20, 2023

Rework the sync async algorithms based on PR 323 #329

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Transfer the input and output views for asynchronous execution #323

Transfer the input and output views for asynchronous execution #323

huningxin commented Jan 6, 2023 •

edited by pr-preview bot

Loading

anssiko commented Jan 9, 2023

wchao1115 left a comment

wchao1115 Jan 10, 2023

huningxin Jan 10, 2023

wchao1115 Jan 10, 2023

huningxin Jan 10, 2023

wchao1115 Jan 10, 2023

huningxin Jan 10, 2023

huningxin commented Jan 10, 2023

domenic left a comment

domenic Jan 10, 2023

huningxin Jan 11, 2023 •

edited

Loading

domenic Jan 10, 2023

huningxin Jan 11, 2023

domenic Jan 10, 2023

huningxin Jan 11, 2023

anssiko commented Jan 10, 2023

huningxin commented Jan 11, 2023

zolkis commented Jan 11, 2023

anssiko left a comment

anssiko commented Jan 13, 2023 •

edited

Loading

anssiko commented Jan 18, 2023


		console.log('Output value: ' + outputBuffer);
		console.log('Output value: ' + result.output);

Transfer the input and output views for asynchronous execution #323

Transfer the input and output views for asynchronous execution #323

Conversation

huningxin commented Jan 6, 2023 • edited by pr-preview bot Loading

anssiko commented Jan 9, 2023

wchao1115 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

huningxin commented Jan 10, 2023

domenic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

huningxin Jan 11, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

anssiko commented Jan 10, 2023

huningxin commented Jan 11, 2023

zolkis commented Jan 11, 2023

anssiko left a comment

Choose a reason for hiding this comment

anssiko commented Jan 13, 2023 • edited Loading

anssiko commented Jan 18, 2023

huningxin commented Jan 6, 2023 •

edited by pr-preview bot

Loading

huningxin Jan 11, 2023 •

edited

Loading

anssiko commented Jan 13, 2023 •

edited

Loading