WebNN: a single big CL to prototype XNNPACK backend [chromium/src : main]

ningxin hu (Gerrit)

unread,

Jun 1, 2022, 10:12:45 PMJun 1

to blink-revie...@chromium.org, blink-...@chromium.org, jmedle...@chromium.org, Honglin Yu, Junwei Fu, Jonathan Bingham, Rafael Cintron, chromium...@chromium.org, Kentaro Hara

Attention is currently required from: Honglin Yu, Junwei Fu.

View Change

To view, visit change 3684745. To unsubscribe, or for help writing mail filters, visit settings.

ningxin hu (Gerrit)

unread,

Jun 1, 2022, 10:20:41 PMJun 1

to blink-revie...@chromium.org, blink-...@chromium.org, jmedle...@chromium.org, Robert Ogden, Alex Gough, Honglin Yu, Junwei Fu, Jonathan Bingham, Rafael Cintron, chromium...@chromium.org, Kentaro Hara

Attention is currently required from: Honglin Yu, Junwei Fu.

View Change

1 comment:

Patchset:
- Patch Set #4:
  CC+ Robert for XNNPACK, Alex for security

To view, visit change 3684745. To unsubscribe, or for help writing mail filters, visit settings.

Junwei Fu (Gerrit)

unread,

Jun 2, 2022, 3:06:50 AMJun 2

to ningxin hu, blink-revie...@chromium.org, blink-...@chromium.org, jmedle...@chromium.org, Robert Ogden, Alex Gough, Honglin Yu, Jonathan Bingham, Rafael Cintron, chromium...@chromium.org, Kentaro Hara

Attention is currently required from: Honglin Yu, ningxin hu.

View Change

10 comments:

Patchset:
- Patch Set #4:
  Exciting implementation, look forward to using WebNN API in browser.
File third_party/blink/renderer/modules/ml/ml_context_xnnpack.h:
- Patch Set #4, Line 20: MLContextXnnpack(const unsigned int num_threads, ML* ml);
  "unsigned int" doesn't need "const" that is temporary value.
File third_party/blink/renderer/modules/ml/ml_context_xnnpack.cc:
- Patch Set #4, Line 39: #if BUILDFLAG(IS_WIN)
  Include the head file like #include "build/buildflag.h"
File third_party/blink/renderer/modules/ml/webnn/ml_graph_builder.cc:
- Patch Set #4, Line 583: Vector<int32_t> output_shape;
  Commbine L.584 - 587 to one line
  Vector<int32_t> output_shape = nchw? {} : {};
- Patch Set #4, Line 592: pool2d->Inputs()[0] = input;
  Does use push_back instead of array like
  pool2d->Inputs().reserve(1);
  pool2d->Inputs().push_back(input);
  or wrap it the two line as a function in the MLOperator.h like
  pool2d->SetInputs({input});
- Patch Set #4, Line 598: pool2d->Outputs().resize(1);
  Ditto.
File third_party/blink/renderer/modules/ml/webnn/ml_graph_xnnpack.h:
- Patch Set #4, Line 18: class MLGraphXnnpack : public MLGraph {
  Add "final" for the "MLGraphXnnpack"
File third_party/blink/renderer/modules/ml/webnn/ml_graph_xnnpack.cc:
- Patch Set #4, Line 172: for (const auto& op : sorted_operators) {
  WebNN has lots of operators, there will be more and more code here, and GPU implementation also need to add the iteration, so do you plan to move it to base class "MLGraph.h", and add a common interface for each operator to implement like https://github.com/webmachinelearning/webnn-native/tree/main/src/webnn/native/ops. thanks.
- Patch Set #4, Line 813: if (inputs.size() != inputs_info_.size()) {
  It's better to reserve the capability for vector with add external_values.reserve(inputs.size()) and move to L.823 before usage.
File third_party/blink/renderer/modules/ml/webnn/ml_operator.cc:
- Patch Set #4, Line 7: #include "third_party/blink/renderer/modules/ml/ml_context.h"
  Remove the unused "ml_context.h" and "ml_graph.h"

To view, visit change 3684745. To unsubscribe, or for help writing mail filters, visit settings.

Robert Ogden (Gerrit)

unread,

Jun 2, 2022, 2:20:07 PMJun 2

to ningxin hu, blink-revie...@chromium.org, blink-...@chromium.org, jmedle...@chromium.org, Alex Gough, Honglin Yu, Junwei Fu, Jonathan Bingham, Rafael Cintron, chromium...@chromium.org, Kentaro Hara

Attention is currently required from: Honglin Yu, ningxin hu.

View Change

1 comment:

Commit Message:
- Patch Set #4, Line 11: In particular, on Linux, this prototype initializes XNNPACK before entering sandbox because XNNPACK requires to access /proc/cpuinfo that is not allowed within sandbox.
  can you point me at where this is done in this CL? I'm trying to understand it

To view, visit change 3684745. To unsubscribe, or for help writing mail filters, visit settings.

Alex Gough (Gerrit)

unread,

Jun 2, 2022, 2:45:53 PMJun 2

to ningxin hu, blink-revie...@chromium.org, blink-...@chromium.org, jmedle...@chromium.org, Alex Gough, Robert Ogden, Honglin Yu, Junwei Fu, Jonathan Bingham, Rafael Cintron, chromium...@chromium.org, Kentaro Hara

Attention is currently required from: Honglin Yu, ningxin hu.

View Change

3 comments:

Patchset:
- Patch Set #4:
  There's a lot of size/shape manipulation going on here - it would be good to have a better story about how these sizes are validated. I've pointed to a couple of locations but this should be considered throughout - a good approach is to use checked casts, spans & base numerics.
File third_party/blink/renderer/modules/ml/webnn/ml_graph_xnnpack.cc:
- Patch Set #4, Line 105: return elements * GetBytesPerElement(operand->Type());
  can this overflow?
- Patch Set #4, Line 126: static_cast<uint32_t>(named_outputs.size() + inputs.size());
  can this overflow? might be good to use checked casts for web-content controlled sizes (throughout)

To view, visit change 3684745. To unsubscribe, or for help writing mail filters, visit settings.

Honglin Yu (Gerrit)

unread,

Jun 3, 2022, 1:57:06 AMJun 3

to ningxin hu, blink-revie...@chromium.org, blink-...@chromium.org, jmedle...@chromium.org, Alex Gough, Robert Ogden, Junwei Fu, Jonathan Bingham, Rafael Cintron, chromium...@chromium.org, Kentaro Hara

Attention is currently required from: ningxin hu.

View Change

10 comments:

Patchset:
- Patch Set #4:
  Thanks for the work! Left a few comments. I feel that maybe we should put the real impl code into somewhere like //components or //third_party/webnn let the blink part mainly focus on web interfaces, especially considering that the code will keep growing when more and more OPs are added.
File third_party/blink/renderer/modules/ml/ml_context_xnnpack.cc:
- Patch Set #4, Line 21: instance_ = refptr.get();
  nit: it feels better to set `instance_` in the constructor by `instance_ = this`.
- Patch Set #4, Line 17:
```
  static scoped_refptr<SharedXnnpackContext> GetInstance(size_t num_threads) {
    if (instance_ == nullptr) {
      scoped_refptr<SharedXnnpackContext> refptr =
          base::MakeRefCounted<SharedXnnpackContext>(num_threads);
      instance_ = refptr.get();
      return refptr;
    } else {
      return base::WrapRefCounted(instance_);
    }
  }
```
  We may need multiple `SharedXnnpackContext` with different configurations (i.e. num_threads here) right? Maybe we need a dictionary of it.
- Patch Set #4, Line 63: WTF::MutexLocker locker(mutex_);
  Is it necessary? i.e. is it possible that the destructor is called more than once? (Would someone explicit call it? Would it be an error to do so?)
- Patch Set #4, Line 67: pthreadpool_
  If the destructor can be called twice, setting `pthreadpool_` to nullptr is needed.
- Patch Set #4, Line 75: WTF::Mutex mutex_;
  Just want to point out that, normally TaskRunner, PostTask and SequenceChecker etc. are prefered than directlly using mutex.
  Also, even when a mutex is needed, it seems `base::Lock` is prefered now: https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/platform/wtf/threading_primitives.h;l=55
File third_party/blink/renderer/modules/ml/webnn/ml_graph.h:
- Patch Set #4, Line 53: ComputeImpl
  nit: this can be private.
File third_party/blink/renderer/modules/ml/webnn/ml_graph_builder.cc:
- Patch Set #4, Line 40: rank_a - i - 1
  this may be negative?
- Patch Set #4, Line 58:
```
int32_t out_size = (input_size + stride - 1) / stride;
  int32_t dilated_filter = (filter_size - 1) * dilation + 1;
  int32_t needed_input = (out_size - 1) * stride + dilated_filter;
```
  Also highlight some places that may overflow.
- Patch Set #4, Line 62: needed_input - input_size
  this can also overflow

To view, visit change 3684745. To unsubscribe, or for help writing mail filters, visit settings.

ningxin hu (Gerrit)

unread,

Jun 3, 2022, 11:24:32 AMJun 3

to blink-revie...@chromium.org, blink-...@chromium.org, jmedle...@chromium.org, Alex Gough, Robert Ogden, Honglin Yu, Junwei Fu, Jonathan Bingham, Rafael Cintron, chromium...@chromium.org, Kentaro Hara

Attention is currently required from: Robert Ogden.

View Change

2 comments:

Commit Message:

- Patch Set #4, Line 11: In particular, on Linux, this prototype initializes XNNPACK before entering sandbox because XNNPACK requires to access /proc/cpuinfo that is not allowed within sandbox.
  can you point me at where this is done in this CL? I'm trying to understand it

- Sure, it is in https://chromium-review.googlesource.com/c/chromium/src/+/3684745/4/chrome/app/chrome_main_delegate.cc
Patchset:
- Patch Set #4:
  Thanks Honglin, Alex, Robert and Junwei for your comments, I'll address them early next week.

To view, visit change 3684745. To unsubscribe, or for help writing mail filters, visit settings.

ningxin hu (Gerrit)

unread,

Jun 5, 2022, 10:35:34 PMJun 5

to blink-revie...@chromium.org, blink-...@chromium.org, jmedle...@chromium.org, Robert Ogden, Alex Gough, Honglin Yu, Junwei Fu, Jonathan Bingham, Rafael Cintron, chromium...@chromium.org, Kentaro Hara

Attention is currently required from: Robert Ogden.

View Change

1 comment:

File third_party/pthreadpool/BUILD.gn:
- Patch Set #4, Line 22:
  There is an issue to build XNNPACK on Windows: https://bugs.chromium.org/p/chromium/issues/detail?id=1333353
  This change is to fix the build issue of pthreadpool. There is another PR to xnnpack: https://github.com/google/XNNPACK/pull/3097. Please take a look as well.

To view, visit change 3684745. To unsubscribe, or for help writing mail filters, visit settings.

ningxin hu (Gerrit)

unread,

Jun 6, 2022, 7:28:59 AMJun 6

to blink-revie...@chromium.org, blink-...@chromium.org, jmedle...@chromium.org, Robert Ogden, Alex Gough, Honglin Yu, Junwei Fu, Jonathan Bingham, Rafael Cintron, chromium...@chromium.org, Kentaro Hara

Attention is currently required from: Alex Gough, Honglin Yu, Junwei Fu, Robert Ogden.

View Change

23 comments:

Patchset:
- Patch Set #4:
  There's a lot of size/shape manipulation going on here - it would be good to have a better story abo […]
  That's great suggestion. I'll look into base numerics and apply to the implementation.
- Patch Set #4:
  Thanks for the work! Left a few comments. […]
  Besides xnnpack backend, the implementation of ops of `MLGraphBuilder` focuses on inputs validation and shapes calculation. As they are (and to be) documented in WebNN spec, the blink implementation seems straight forward to me. If there is a demand to be shared with other components, I'll move it. Please let me know.
Patchset:
- Patch Set #5:
  Fixed Honglin and Junwei's comments except shape calculation by using base numerics (also Alex's comment). I'll fix that in the following patchset.
File third_party/blink/renderer/modules/ml/ml_context_xnnpack.h:

- Patch Set #4, Line 20: MLContextXnnpack(const unsigned int num_threads, ML* ml);
  "unsigned int" doesn't need "const" that is temporary value.

- The num_threads support is removed.
File third_party/blink/renderer/modules/ml/ml_context_xnnpack.cc:

- Patch Set #4, Line 21: instance_ = refptr.get();
  nit: it feels better to set `instance_` in the constructor by `instance_ = this`.

- Ack

Patch Set #4, Line 17:

  static scoped_refptr<SharedXnnpackContext> GetInstance(size_t num_threads) {
    if (instance_ == nullptr) {
      scoped_refptr<SharedXnnpackContext> refptr =
          base::MakeRefCounted<SharedXnnpackContext>(num_threads);
      instance_ = refptr.get();
      return refptr;
    } else {
      return base::WrapRefCounted(instance_);
    }
  }

- We may need multiple `SharedXnnpackContext` with different configurations (i.e. […]
  You are right. However, according to webnn spec, there is no num_threads option. It was here only for testing purpose. I'll remove that and we only need single `SharedXnnpackContext`.

- Patch Set #4, Line 39: #if BUILDFLAG(IS_WIN)

- Include the head file like #include "build/buildflag. […]
  Ack

- Patch Set #4, Line 63: WTF::MutexLocker locker(mutex_);

- Is it necessary? i.e. […]
  It's unnecessary. It should be only called once. I'll fix it.

- Patch Set #4, Line 67: pthreadpool_
  If the destructor can be called twice, setting `pthreadpool_` to nullptr is needed.

- It should be called once. I'll fix above comment.

- Patch Set #4, Line 75: WTF::Mutex mutex_;

- Just want to point out that, normally TaskRunner, PostTask and SequenceChecker etc. […]
  Ack
File third_party/blink/renderer/modules/ml/webnn/ml_graph.h:

- Patch Set #4, Line 53: ComputeImpl
  nit: this can be private.

- Ack
File third_party/blink/renderer/modules/ml/webnn/ml_graph_builder.cc:

- Patch Set #4, Line 40: rank_a - i - 1
  this may be negative?

- ack. I'll fix it by using base numerics as Alex suggested.

Patch Set #4, Line 58:

int32_t out_size = (input_size + stride - 1) / stride;
  int32_t dilated_filter = (filter_size - 1) * dilation + 1;
  int32_t needed_input = (out_size - 1) * stride + dilated_filter;

Also highlight some places that may overflow.

- ditto

- Patch Set #4, Line 62: needed_input - input_size
  this can also overflow

- ditto.

- Patch Set #4, Line 583: Vector<int32_t> output_shape;

- Commbine L.584 - 587 to one line […]
  This would cause a compilation error: initializer list cannot be used on the right hand side of operator '?'

- Patch Set #4, Line 592: pool2d->Inputs()[0] = input;

- Does use push_back instead of array like […]
  Ack

- Patch Set #4, Line 598: pool2d->Outputs().resize(1);
  Ditto.

- Ack
File third_party/blink/renderer/modules/ml/webnn/ml_graph_xnnpack.h:

- Patch Set #4, Line 18: class MLGraphXnnpack : public MLGraph {
  Add "final" for the "MLGraphXnnpack"

- Ack
File third_party/blink/renderer/modules/ml/webnn/ml_graph_xnnpack.cc:

- Patch Set #4, Line 105: return elements * GetBytesPerElement(operand->Type());
  can this overflow?

- Ack, will fix in the following patchset.

- Patch Set #4, Line 126: static_cast<uint32_t>(named_outputs.size() + inputs.size());
  can this overflow? might be good to use checked casts for web-content controlled sizes (throughout)

- ditto.

- Patch Set #4, Line 172: for (const auto& op : sorted_operators) {

- WebNN has lots of operators, there will be more and more code here, and GPU implementation also need […]
  Thanks for the suggestion. For this CL, I'd like to consolidate the graph builder code that is easier for review.

- Patch Set #4, Line 813: if (inputs.size() != inputs_info_.size()) {

- It's better to reserve the capability for vector with add external_values.reserve(inputs. […]
  Ack
File third_party/blink/renderer/modules/ml/webnn/ml_operator.cc:

- Patch Set #4, Line 7: #include "third_party/blink/renderer/modules/ml/ml_context.h"

- Remove the unused "ml_context.h" and "ml_graph. […]
  Ack

To view, visit change 3684745. To unsubscribe, or for help writing mail filters, visit settings.

ningxin hu (Gerrit)

unread,

Jun 6, 2022, 7:35:25 AMJun 6

to blink-revie...@chromium.org, blink-...@chromium.org, jmedle...@chromium.org, Robert Ogden, Alex Gough, Honglin Yu, Junwei Fu, Jonathan Bingham, Rafael Cintron, chromium...@chromium.org, Kentaro Hara

Attention is currently required from: Alex Gough, Honglin Yu, Junwei Fu, Robert Ogden.

View Change

3 comments:

File third_party/blink/renderer/modules/ml/ml_context_options.idl:
- Patch Set #5, Line 20: "default"
  The latest webnn spec doesn't have this `default`. It should be removed.
- Patch Set #5, Line 32: "default"
  We need to finalize the `auto` or `default` discussion in WG and implement that.
- Patch Set #5, Line 47: MLContextType type = "webnn";
  I feel we need to introduce the `MLContextType` for Model-Loader and WebNN. The current default value is just for running legacy WebNN tests. It should not be set by user explicitly. I'll remove it once we make consensus in WG.
  Honglin, WDYT?

To view, visit change 3684745. To unsubscribe, or for help writing mail filters, visit settings.

Robert Ogden (Gerrit)

unread,

Jun 6, 2022, 12:08:40 PMJun 6

to ningxin hu, blink-revie...@chromium.org, blink-...@chromium.org, jmedle...@chromium.org, Matthew Denton, Alex Gough, Honglin Yu, Junwei Fu, Jonathan Bingham, Rafael Cintron, chromium...@chromium.org, Kentaro Hara

Attention is currently required from: Alex Gough, Honglin Yu, Junwei Fu, ningxin hu.

View Change

1 comment:

Patchset:
- Patch Set #5:
  +mpdenton
  You'll also need to remove this blocked syscall (https://source.chromium.org/chromium/chromium/src/+/main:third_party/cpuinfo/src/src/api.c;l=364?q=f:cpuinfo%20getcpu) here (https://source.chromium.org/chromium/chromium/src/+/main:sandbox/linux/seccomp-bpf-helpers/syscall_sets.cc;l=756;drc=8d6a246c9be4f6b731dc7f6e680b7d5e13a512b5)

To view, visit change 3684745. To unsubscribe, or for help writing mail filters, visit settings.

Robert Ogden (Gerrit)

unread,

Jun 6, 2022, 12:09:23 PMJun 6

to ningxin hu, blink-revie...@chromium.org, blink-...@chromium.org, jmedle...@chromium.org, Matthew Denton, Alex Gough, Honglin Yu, Junwei Fu, Jonathan Bingham, Rafael Cintron, chromium...@chromium.org, Kentaro Hara

Attention is currently required from: Alex Gough, Honglin Yu, Junwei Fu, ningxin hu.

View Change

1 comment:

Commit Message:

- Patch Set #4, Line 11: In particular, on Linux, this prototype initializes XNNPACK before entering sandbox because XNNPACK requires to access /proc/cpuinfo that is not allowed within sandbox.

- Sure, it is in https://chromium-review.googlesource. […]
  resolved

To view, visit change 3684745. To unsubscribe, or for help writing mail filters, visit settings.

Matthew Denton (Gerrit)

unread,

Jun 6, 2022, 3:45:22 PMJun 6

to ningxin hu, blink-revie...@chromium.org, blink-...@chromium.org, jmedle...@chromium.org, Robert Ogden, Alex Gough, Honglin Yu, Junwei Fu, Jonathan Bingham, Rafael Cintron, chromium...@chromium.org, Kentaro Hara

Attention is currently required from: Alex Gough, Honglin Yu, Junwei Fu, Robert Ogden, ningxin hu.

View Change

2 comments:

Patchset:
- Patch Set #5:
  +mpdenton […]
  Should I do this now or is this just a prototype?
File chrome/app/chrome_main_delegate.cc:
- Patch Set #5, Line 1070: xnn_initialize
  We only want this in the renderer right? Let's maybe put it in here? https://source.chromium.org/chromium/chromium/src/+/main:content/renderer/renderer_main_platform_delegate_linux.cc;bpv=1;bpt=1;l=26?gsn=PlatformInitialize&gs=kythe%3A%2F%2Fchromium.googlesource.com%2Fchromium%2Fsrc%3Flang%3Dc%252B%252B%3Fpath%3Dcontent%2Frenderer%2Frenderer_main_platform_delegate.h%23GunO2Kw9p9mafLKbvzWZtupqEKlS-KJu4FBfZn-IK7o&gs=kythe%3A%2F%2Fchromium.googlesource.com%2Fchromium%2Fsrc%3Flang%3Dc%252B%252B%3Fpath%3Dcontent%2Frenderer%2Frenderer_main_platform_delegate_linux.cc%23ML4tv-sy2slamaF8osmvEsIfRJzSb0q5L4LeBYbVZMA

To view, visit change 3684745. To unsubscribe, or for help writing mail filters, visit settings.

ningxin hu (Gerrit)

unread,

Jun 6, 2022, 11:34:55 PMJun 6

to blink-revie...@chromium.org, blink-...@chromium.org, jmedle...@chromium.org, Matthew Denton, Robert Ogden, Alex Gough, Honglin Yu, Junwei Fu, Jonathan Bingham, Rafael Cintron, chromium...@chromium.org, Kentaro Hara

Attention is currently required from: Alex Gough, Honglin Yu, Junwei Fu, Matthew Denton, Robert Ogden.

View Change

1 comment:

File chrome/app/chrome_main_delegate.cc:

- Patch Set #5, Line 1070: xnn_initialize

- We only want this in the renderer right? Let's maybe put it in here? https://source.chromium. […]
  Thanks Matthew. I tried to move the `xnn_initialize` to renderer_main_platform_delegate_linux.cc, however it reports error about parsing /proc/cpuinfo as log shows:
  "
  Error in cpuinfo: failed to parse processor information from /proc/cpuinfo
  [24807:1:0607/110359.588421:ERROR:renderer_main_platform_delegate_linux.cc(30)] Failed to initialize XNNPACK
  "
  Did I miss anything?

To view, visit change 3684745. To unsubscribe, or for help writing mail filters, visit settings.

ningxin hu (Gerrit)

unread,

Jun 7, 2022, 2:20:02 AMJun 7

to blink-revie...@chromium.org, blink-...@chromium.org, jmedle...@chromium.org, Matthew Denton, Robert Ogden, Alex Gough, Honglin Yu, Junwei Fu, Jonathan Bingham, Rafael Cintron, chromium...@chromium.org, Kentaro Hara

Attention is currently required from: Alex Gough, Honglin Yu, Junwei Fu, Matthew Denton, Robert Ogden.

View Change

1 comment:

File chrome/app/chrome_main_delegate.cc:

- Patch Set #5, Line 1070: xnn_initialize

- Thanks Matthew. I tried to move the `xnn_initialize` to renderer_main_platform_delegate_linux. […]
  Patchset6 reflects what I reported.

To view, visit change 3684745. To unsubscribe, or for help writing mail filters, visit settings.

Matthew Denton (Gerrit)

unread,

Jun 7, 2022, 4:29:20 PMJun 7

to ningxin hu, blink-revie...@chromium.org, blink-...@chromium.org, jmedle...@chromium.org, Robert Ogden, Alex Gough, Honglin Yu, Junwei Fu, Jonathan Bingham, Rafael Cintron, chromium...@chromium.org, Kentaro Hara

Attention is currently required from: Alex Gough, Honglin Yu, Junwei Fu, Robert Ogden, ningxin hu.

View Change

1 comment:

File chrome/app/chrome_main_delegate.cc:

- Patch Set #5, Line 1070: xnn_initialize

- Patchset6 reflects what I reported.
  Ah sorry, it seems a decent amount of this process startup code (in particular, renderer_main_platform_delegate.h) actually predates the first Linux version of Chrome, so the comments are a bit off... You probably want it in here: https://source.chromium.org/chromium/chromium/src/+/main:content/app/content_main_runner_impl.cc;drc=1946212ac0100668f14eb9e2843bdd846e510a1e;bpv=1;bpt=1;l=400?gsn=PreSandboxInit&gs=kythe%3A%2F%2Fchromium.googlesource.com%2Fchromium%2Fsrc%3Flang%3Dc%252B%252B%3Fpath%3Dcontent%2Fapp%2Fcontent_main_runner_impl.cc%23Sc7VDw2LzzmuolcMyG3lNcwW5-z0_zz4fSQvoM4kpYs
  This initializes xnn in the zygote before the zygote's partial sandbox is applied.

To view, visit change 3684745. To unsubscribe, or for help writing mail filters, visit settings.

ningxin hu (Gerrit)

unread,

Jun 8, 2022, 2:43:38 AMJun 8

to blink-revie...@chromium.org, blink-...@chromium.org, jmedle...@chromium.org, Matthew Denton, Robert Ogden, Alex Gough, Honglin Yu, Junwei Fu, Jonathan Bingham, Rafael Cintron, chromium...@chromium.org, Kentaro Hara

Attention is currently required from: Alex Gough, Honglin Yu, Junwei Fu, Matthew Denton, Robert Ogden.

View Change

8 comments:

Patchset:
- Patch Set #4:
  That's great suggestion. I'll look into base numerics and apply to the implementation.
  Applied checked casts and base numerics in Pathset 7. Please take another look. Thanks.
Patchset:
- Patch Set #5:

- Should I do this now or is this just a prototype?

- This is for WebNN intent-to-prototype: https://groups.google.com/a/chromium.org/g/blink-dev/c/PD6TDMDS9mg?pli=1
File chrome/app/chrome_main_delegate.cc:

- Patch Set #5, Line 1070: xnn_initialize

- Ah sorry, it seems a decent amount of this process startup code (in particular, renderer_main_platfo […]
  Yeah, it works. It is implemented in Patchset 7. Please take another look. Thanks.
File third_party/blink/renderer/modules/ml/webnn/ml_graph_builder.cc:

- Patch Set #4, Line 40: rank_a - i - 1

- ack. I'll fix it by using base numerics as Alex suggested.
  I understand this would not be negative, because `i < rank_a` is already hold before, correct?

Patch Set #4, Line 58:

int32_t out_size = (input_size + stride - 1) / stride;
  int32_t dilated_filter = (filter_size - 1) * dilation + 1;
  int32_t needed_input = (out_size - 1) * stride + dilated_filter;

- ditto
  Fixed in Pathset 7 by using checked math. Please take another look. Thanks.

- Patch Set #4, Line 62: needed_input - input_size

- ditto.
  Done
File third_party/blink/renderer/modules/ml/webnn/ml_graph_xnnpack.cc:

- Patch Set #4, Line 105: return elements * GetBytesPerElement(operand->Type());

- Ack, will fix in the following patchset.
  Done

- Patch Set #4, Line 126: static_cast<uint32_t>(named_outputs.size() + inputs.size());

- ditto.
  Done

To view, visit change 3684745. To unsubscribe, or for help writing mail filters, visit settings.

ningxin hu (Gerrit)

unread,

Jun 8, 2022, 4:17:18 AMJun 8

to blink-revie...@chromium.org, blink-...@chromium.org, jmedle...@chromium.org, Matthew Denton, Robert Ogden, Alex Gough, Honglin Yu, Junwei Fu, Jonathan Bingham, Rafael Cintron, chromium...@chromium.org, Kentaro Hara

Attention is currently required from: Alex Gough, Honglin Yu, Junwei Fu, Matthew Denton, Robert Ogden.

View Change

1 comment:

File third_party/blink/renderer/modules/ml/ml.idl:
- Patch Set #7, Line 16: MLContext createContext(optional MLContextOptions options = {});
  TFLite WebNN delegate C++ implementation requires sync call. https://github.com/huningxin/tensorflow/blob/webnn_delegate/tensorflow/lite/delegates/webnn/webnn_delegate.cc#L138
  Probably we need to define both sync and async version. This is another spec open.
  Any thoughts, Honglin?

To view, visit change 3684745. To unsubscribe, or for help writing mail filters, visit settings.

Honglin Yu (Gerrit)

unread,

Jun 8, 2022, 5:23:43 AMJun 8

to ningxin hu, blink-revie...@chromium.org, blink-...@chromium.org, jmedle...@chromium.org, Matthew Denton, Robert Ogden, Alex Gough, Junwei Fu, Jonathan Bingham, Rafael Cintron, chromium...@chromium.org, Kentaro Hara

Attention is currently required from: Alex Gough, Junwei Fu, Matthew Denton, Robert Ogden, ningxin hu.

View Change

1 comment:

File third_party/blink/renderer/modules/ml/ml.idl:

- Patch Set #7, Line 16: MLContext createContext(optional MLContextOptions options = {});

- TFLite WebNN delegate C++ implementation requires sync call. https://github. […]
  Yeah, having two versions sounds fine to me, at least for the moment.

To view, visit change 3684745. To unsubscribe, or for help writing mail filters, visit settings.

Robert Ogden (Gerrit)

unread,

Jun 8, 2022, 6:53:56 PMJun 8

to ningxin hu, blink-revie...@chromium.org, blink-...@chromium.org, jmedle...@chromium.org, Matthew Denton, Alex Gough, Honglin Yu, Junwei Fu, Jonathan Bingham, Rafael Cintron, chromium...@chromium.org, Kentaro Hara

Attention is currently required from: Alex Gough, Junwei Fu, Matthew Denton, ningxin hu.

View Change

1 comment:

Patchset:
- Patch Set #5:
  This is for WebNN intent-to-prototype: https://groups.google.com/a/chromium. […]
  The syscall is being allowed here, https://crrev.com/c/3677495, so no action needed anymore

To view, visit change 3684745. To unsubscribe, or for help writing mail filters, visit settings.

Honglin Yu (Gerrit)

unread,

Jun 8, 2022, 9:06:36 PMJun 8

to ningxin hu, blink-revie...@chromium.org, blink-...@chromium.org, jmedle...@chromium.org, Matthew Denton, Robert Ogden, Alex Gough, Junwei Fu, Jonathan Bingham, Rafael Cintron, chromium...@chromium.org, Kentaro Hara

Attention is currently required from: Alex Gough, Junwei Fu, Matthew Denton, ningxin hu.

View Change

1 comment:

File third_party/blink/renderer/modules/ml/ml.idl:

- Patch Set #7, Line 16: MLContext createContext(optional MLContextOptions options = {});

- Yeah, having two versions sounds fine to me, at least for the moment.
  It seems more natural to have the "async" version as default and create a new function `createContextSync` for the sync version. The reason is that from
  1. https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/web_tests/platform/generic/webexposed/global-interface-listing-expected.txt
  2. https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/web_tests/platform/generic/virtual/stable/webexposed/global-interface-listing-expected.txt
  there are functions whose names have postfix "Sync" but there is none having postfix "Sync".
  What do you think?

To view, visit change 3684745. To unsubscribe, or for help writing mail filters, visit settings.

Alex Gough (Gerrit)

unread,

Jun 8, 2022, 9:57:38 PMJun 8

to ningxin hu, blink-revie...@chromium.org, blink-...@chromium.org, jmedle...@chromium.org, Matthew Denton, Robert Ogden, Alex Gough, Honglin Yu, Junwei Fu, Jonathan Bingham, Rafael Cintron, chromium...@chromium.org, Kentaro Hara

Attention is currently required from: Junwei Fu, Matthew Denton, ningxin hu.

View Change

1 comment:

Patchset:

Patch Set #7:

Note: I was ill today so did not get to take another look and I'll be OOO for the next few days. Feel free to wait until mid-next-week or swap for a different 
 reviewer.

To view, visit change 3684745. To unsubscribe, or for help writing mail filters, visit settings.

Honglin Yu (Gerrit)

unread,

Jun 8, 2022, 10:07:39 PMJun 8

to ningxin hu, blink-revie...@chromium.org, blink-...@chromium.org, jmedle...@chromium.org, Matthew Denton, Robert Ogden, Alex Gough, Junwei Fu, Jonathan Bingham, Rafael Cintron, chromium...@chromium.org, Kentaro Hara

Attention is currently required from: Alex Gough, Junwei Fu, Matthew Denton, ningxin hu.

View Change

1 comment:

Patchset:
- Patch Set #7:
  Note: I was ill today so did not get to take another look and I'll be OOO for the next few days. […]
  Sorry to hear it, Alex. Get better soon!

To view, visit change 3684745. To unsubscribe, or for help writing mail filters, visit settings.

ningxin hu (Gerrit)

unread,

Jun 8, 2022, 10:08:54 PMJun 8

to blink-revie...@chromium.org, blink-...@chromium.org, jmedle...@chromium.org, Matthew Denton, Robert Ogden, Alex Gough, Honglin Yu, Junwei Fu, Jonathan Bingham, Rafael Cintron, chromium...@chromium.org, Kentaro Hara

Attention is currently required from: Alex Gough, Junwei Fu, Matthew Denton.

View Change

1 comment:

Patchset:

- Patch Set #7:
  Note: I was ill today so did not get to take another look and I'll be OOO for the next few days. […]

- Alex, no worries, it's no problem to me, and thanks for sharing with me. Please take a good reset and wish you recover soon. At meanwhile, I can continue to address the comments from Honglin on implementation and other spec related items. Take care!

To view, visit change 3684745. To unsubscribe, or for help writing mail filters, visit settings.

ningxin hu (Gerrit)

unread,

Jun 8, 2022, 11:02:58 PMJun 8

to blink-revie...@chromium.org, blink-...@chromium.org, jmedle...@chromium.org, Matthew Denton, Robert Ogden, Alex Gough, Honglin Yu, Junwei Fu, Jonathan Bingham, Rafael Cintron, chromium...@chromium.org, Kentaro Hara

Attention is currently required from: Alex Gough, Honglin Yu, Junwei Fu, Matthew Denton.

View Change

1 comment:

File third_party/blink/renderer/modules/ml/ml.idl:

- Patch Set #7, Line 16: MLContext createContext(optional MLContextOptions options = {});

- It seems more natural to have the "async" version as default and create a new function `createContex […]
  The current WebNN spec has "Async" postfix for async version, e.g., `MLGraphBuilder.buildAsync`. WebGPU API also share this convention, where if an API has both sync and async versions, the async one has "Async" postfix. e.g., `GPUDevice.createComputePipelineAsync`: https://www.w3.org/TR/webgpu/#dom-gpudevice-createcomputepipelineasync
  As you shared, there is another convention vise-versa that has postfix "Sync" for sync version.
  I am fine with both conventions. I think we need to fix:
  1. Introduce the async version of `createContext` into WebNN spec.
  2. Align the sync and async methods naming within WebNN spec and with Model-Loader spec.
  I propose to discuss those two items within WebML WG. WDYT?

To view, visit change 3684745. To unsubscribe, or for help writing mail filters, visit settings.

Honglin Yu (Gerrit)

unread,

Jun 8, 2022, 11:06:17 PMJun 8

to ningxin hu, blink-revie...@chromium.org, blink-...@chromium.org, jmedle...@chromium.org, Matthew Denton, Robert Ogden, Alex Gough, Junwei Fu, Jonathan Bingham, Rafael Cintron, chromium...@chromium.org, Kentaro Hara

Attention is currently required from: Alex Gough, Junwei Fu, Matthew Denton, ningxin hu.

View Change

1 comment:

File third_party/blink/renderer/modules/ml/ml.idl:

- Patch Set #7, Line 16: MLContext createContext(optional MLContextOptions options = {});

- The current WebNN spec has "Async" postfix for async version, e.g., `MLGraphBuilder.buildAsync`. […]
  I see. If that is the case, I don't have any preference either. And agreed, let's discuss further in the future. Thanks

To view, visit change 3684745. To unsubscribe, or for help writing mail filters, visit settings.

Matthew Denton (Gerrit)

unread,

Jun 9, 2022, 3:51:23 PMJun 9

to ningxin hu, blink-revie...@chromium.org, blink-...@chromium.org, jmedle...@chromium.org, Robert Ogden, Alex Gough, Honglin Yu, Junwei Fu, Jonathan Bingham, Rafael Cintron, chromium...@chromium.org, Kentaro Hara

Attention is currently required from: Alex Gough, Junwei Fu, ningxin hu.

View Change

1 comment:

File content/app/content_main_runner_impl.cc:
- Patch Set #7, Line 414: /cpu/cpuinfo
  third_party/tflite/src/third_party/cpuinfo/

To view, visit change 3684745. To unsubscribe, or for help writing mail filters, visit settings.

Matthew Denton (Gerrit)

unread,

Jun 9, 2022, 3:52:11 PMJun 9

to ningxin hu, blink-revie...@chromium.org, blink-...@chromium.org, jmedle...@chromium.org, Robert Ogden, Alex Gough, Honglin Yu, Junwei Fu, Jonathan Bingham, Rafael Cintron, chromium...@chromium.org, Kentaro Hara

Attention is currently required from: Alex Gough, Junwei Fu, ningxin hu.

View Change

1 comment:

File content/app/content_main_runner_impl.cc:

- Patch Set #7, Line 414: /cpu/cpuinfo
  third_party/tflite/src/third_party/cpuinfo/

- Whoops, ignore this.

To view, visit change 3684745. To unsubscribe, or for help writing mail filters, visit settings.

Junwei (Gerrit)

unread,

Jun 9, 2022, 8:28:41 PMJun 9

to ningxin hu, blink-revie...@chromium.org, blink-...@chromium.org, jmedle...@chromium.org, Matthew Denton, Robert Ogden, Alex Gough, Honglin Yu, Junwei Fu, Jonathan Bingham, Rafael Cintron, chromium...@chromium.org, Kentaro Hara

Attention is currently required from: Alex Gough, Junwei Fu, ningxin hu.

View Change

1 comment:

File third_party/blink/renderer/modules/ml/webnn/ml_graph_builder.cc:
- Patch Set #7, Line 673: while (nodes_to_do.size() > 0) {
  Maybe the algorithm of sorting operators need to extract a common function for async build [1].
  [1] https://webmachinelearning.github.io/webnn/#dom-mlgraphbuilder-buildasync

To view, visit change 3684745. To unsubscribe, or for help writing mail filters, visit settings.

ningxin hu (Gerrit)

unread,

Jun 10, 2022, 4:33:23 AMJun 10

to blink-revie...@chromium.org, blink-...@chromium.org, jmedle...@chromium.org, Junwei, Matthew Denton, Robert Ogden, Alex Gough, Honglin Yu, Junwei Fu, Jonathan Bingham, Rafael Cintron, chromium...@chromium.org, Kentaro Hara

Attention is currently required from: Honglin Yu, Junwei Fu.

View Change

4 comments:

File third_party/blink/renderer/modules/ml/ml_context_options.idl:

- Patch Set #5, Line 20: "default"
  The latest webnn spec doesn't have this `default`. It should be removed.

- Reverted this change in PS9, let's leave it as it until spec discussion closed.

- Patch Set #5, Line 32: "default"
  We need to finalize the `auto` or `default` discussion in WG and implement that.

- Reverted this change in PS9, let's leave it as it until spec discussion closed.

- Patch Set #5, Line 47: MLContextType type = "webnn";

- I feel we need to introduce the `MLContextType` for Model-Loader and WebNN. […]
  I rethink about this design. It turns out this context type is not so necessary. In PS8, I reverted the `MLContextType` and `MLContextXnnpack`, and moved WebNN XNNPACK initialization and pthreadpool code to `MLGraphXnnpack`. This would avoid diverging from the spec.
  Please take another look.
File third_party/blink/renderer/modules/ml/webnn/ml_graph_builder.cc:

- Patch Set #7, Line 673: while (nodes_to_do.size() > 0) {

- Maybe the algorithm of sorting operators need to extract a common function for async build [1]. […]
  Done in PS9, please take a look.

To view, visit change 3684745. To unsubscribe, or for help writing mail filters, visit settings.

ningxin hu (Gerrit)

unread,

Jun 10, 2022, 5:03:05 AMJun 10

to blink-revie...@chromium.org, blink-...@chromium.org, jmedle...@chromium.org, Junwei, Matthew Denton, Robert Ogden, Alex Gough, Honglin Yu, Junwei Fu, Jonathan Bingham, Rafael Cintron, chromium...@chromium.org, Kentaro Hara

Attention is currently required from: Honglin Yu, Robert Ogden.

View Change

2 comments:

File third_party/blink/renderer/modules/ml/ml.idl:

- Patch Set #7, Line 16: MLContext createContext(optional MLContextOptions options = {});

- I see. If that is the case, I don't have any preference either. […]
  https://github.com/webmachinelearning/webnn/issues/272 created, let's see WG's inputs.
  Because the current implementation creates `MLContxt` in the renderer [1], there should be no issue to change `createContext` to sync call. We'd implement the async version once the spec discussion finalized.
  Honglin, WDYT?
  [1]: https://source.chromium.org/chromium/chromium/src/+/main:third_party/blink/renderer/modules/ml/ml.cc;l=59
File third_party/pthreadpool/BUILD.gn:

- Patch Set #4, Line 22:

- There is an issue to build XNNPACK on Windows: https://bugs.chromium. […]
  https://chromium-review.googlesource.com/c/chromium/src/+/3699133 submitted to fix this separately. Once that one merged, I'll remove the fix in this CL.

To view, visit change 3684745. To unsubscribe, or for help writing mail filters, visit settings.

Robert Ogden (Gerrit)

unread,

Jun 10, 2022, 11:44:42 AMJun 10

to ningxin hu, blink-revie...@chromium.org, blink-...@chromium.org, jmedle...@chromium.org, Junwei, Matthew Denton, Alex Gough, Honglin Yu, Junwei Fu, Jonathan Bingham, Rafael Cintron, chromium...@chromium.org, Kentaro Hara

Attention is currently required from: Honglin Yu, ningxin hu.

View Change

1 comment:

File third_party/pthreadpool/BUILD.gn:

- Patch Set #4, Line 22:

- https://chromium-review.googlesource.com/c/chromium/src/+/3699133 submitted to fix this separately. […]
  the XNNPACK PR is included in this roll https://crrev.com/c/3699093
  pthreadpool fix is approved in https://crrev.com/c/3699133
  resolving this comment

To view, visit change 3684745. To unsubscribe, or for help writing mail filters, visit settings.

Robert Ogden (Gerrit)

unread,

Jun 10, 2022, 1:35:13 PMJun 10

to ningxin hu, blink-revie...@chromium.org, blink-...@chromium.org, jmedle...@chromium.org, Junwei, Matthew Denton, Alex Gough, Honglin Yu, Junwei Fu, Jonathan Bingham, Rafael Cintron, chromium...@chromium.org, Kentaro Hara

Attention is currently required from: Honglin Yu, ningxin hu.

Patch set 9:Code-Review +1

View Change

1 comment:

Patchset:
- Patch Set #9:
  https://crrev.com/c/3677495 has landed, so lgtm for the DEPS on XNNPACK.

To view, visit change 3684745. To unsubscribe, or for help writing mail filters, visit settings.

ningxin hu (Gerrit)

unread,

Jun 22, 2022, 11:35:39 PMJun 22

to blink-revie...@chromium.org, blink-...@chromium.org, jmedle...@chromium.org, Robert Ogden, Junwei, Matthew Denton, Alex Gough, Honglin Yu, Junwei Fu, Jonathan Bingham, Rafael Cintron, chromium...@chromium.org, Kentaro Hara

Attention is currently required from: Honglin Yu.

View Change

2 comments:

Patchset:
- Patch Set #12:
  Prototype the async methods and restrict the sync methods in dedicated worker according to the spec change [1]. Please take another look. Thanks.
  [1]: https://github.com/webmachinelearning/webnn/pull/274
File third_party/blink/renderer/modules/ml/ml.idl:

- Patch Set #7, Line 16: MLContext createContext(optional MLContextOptions options = {});

- https://github.com/webmachinelearning/webnn/issues/272 created, let's see WG's inputs. […]
  Implemented the async method in PS12, resolve this comment, thanks.

To view, visit change 3684745. To unsubscribe, or for help writing mail filters, visit settings.

ningxin hu (Gerrit)

unread,

Jun 22, 2022, 11:38:07 PMJun 22

to blink-revie...@chromium.org, blink-...@chromium.org, jmedle...@chromium.org, Chromium LUCI CQ, Robert Ogden, Junwei, Matthew Denton, Alex Gough, Honglin Yu, Junwei Fu, Jonathan Bingham, Rafael Cintron, chromium...@chromium.org, Kentaro Hara

Attention is currently required from: Honglin Yu.

View Change

1 comment:

Patchset:
- Patch Set #12:
  Honglin, could you please help trigger the try bots? Thanks.

To view, visit change 3684745. To unsubscribe, or for help writing mail filters, visit settings.

Honglin Yu (Gerrit)

unread,

Jun 22, 2022, 11:41:15 PMJun 22

to ningxin hu, blink-revie...@chromium.org, blink-...@chromium.org, jmedle...@chromium.org, Chromium LUCI CQ, Robert Ogden, Junwei, Matthew Denton, Alex Gough, Junwei Fu, Jonathan Bingham, Rafael Cintron, chromium...@chromium.org, Kentaro Hara

Attention is currently required from: ningxin hu.

Patch set 12:Commit-Queue +1

View Change

1 comment:

Patchset:

- Patch Set #12:
  Honglin, could you please help trigger the try bots? Thanks.

- Done.

To view, visit change 3684745. To unsubscribe, or for help writing mail filters, visit settings.

ningxin hu (Gerrit)

unread,

Jun 23, 2022, 2:55:29 AMJun 23

to blink-revie...@chromium.org, blink-...@chromium.org, jmedle...@chromium.org, Honglin Yu, Chromium LUCI CQ, Robert Ogden, Junwei, Matthew Denton, Alex Gough, Junwei Fu, Jonathan Bingham, Rafael Cintron, chromium...@chromium.org, Kentaro Hara

Attention is currently required from: Honglin Yu.

View Change

1 comment:

Patchset:
- Patch Set #13:
  Honglin, I fixed the build deps check failure in PS13, could you please help trigger another run? Thanks!
  BTW, how could I get the permission, so I can run trybots by myself without interfering you?

To view, visit change 3684745. To unsubscribe, or for help writing mail filters, visit settings.

Honglin Yu (Gerrit)

unread,

Jun 23, 2022, 3:03:15 AMJun 23

to ningxin hu, blink-revie...@chromium.org, blink-...@chromium.org, jmedle...@chromium.org, Chromium LUCI CQ, Robert Ogden, Junwei, Matthew Denton, Alex Gough, Junwei Fu, Jonathan Bingham, Rafael Cintron, chromium...@chromium.org, Kentaro Hara

Attention is currently required from: ningxin hu.

Patch set 13:Commit-Queue +1

View Change

1 comment:

Patchset:
- Patch Set #13:
  Honglin, I fixed the build deps check failure in PS13, could you please help trigger another run? Th […]
  How to have tryjob access: https://www.chromium.org/getting-involved/become-a-committer/#try-job-access. I can help send email to acco...@chromium.org if you like.

To view, visit change 3684745. To unsubscribe, or for help writing mail filters, visit settings.

Honglin Yu (Gerrit)

unread,

Jun 23, 2022, 3:03:46 AMJun 23

to ningxin hu, blink-revie...@chromium.org, blink-...@chromium.org, jmedle...@chromium.org, Chromium LUCI CQ, Robert Ogden, Junwei, Matthew Denton, Alex Gough, Junwei Fu, Jonathan Bingham, Rafael Cintron, chromium...@chromium.org, Kentaro Hara

Attention is currently required from: ningxin hu.

View Change

1 comment:

Patchset:
- Patch Set #13:
  How to have tryjob access: https://www.chromium. […]
  And no problem at all.

To view, visit change 3684745. To unsubscribe, or for help writing mail filters, visit settings.

ningxin hu (Gerrit)

unread,

Jun 23, 2022, 3:11:00 AMJun 23