WebNN: a single big CL to prototype XNNPACK backend [chromium/src : main]
ningxin hu (Gerrit)
Attention is currently required from: Honglin Yu, Junwei Fu.
To view, visit change 3684745. To unsubscribe, or for help writing mail filters, visit settings.
ningxin hu (Gerrit)
Attention is currently required from: Honglin Yu, Junwei Fu.
1 comment:
Patchset:
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)
Attention is currently required from: Honglin Yu, ningxin hu.
10 comments:
Patchset:
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)
Attention is currently required from: Honglin Yu, ningxin hu.
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)
Attention is currently required from: Honglin Yu, ningxin hu.
3 comments:
Patchset:
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)
Attention is currently required from: ningxin hu.
10 comments:
Patchset:
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`.
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:
ComputeImplnit: this can be private.
File third_party/blink/renderer/modules/ml/webnn/ml_graph_builder.cc:
Patch Set #4, Line 40:
rank_a - i - 1this may be negative?
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_sizethis can also overflow
To view, visit change 3684745. To unsubscribe, or for help writing mail filters, visit settings.
ningxin hu (Gerrit)
Attention is currently required from: Robert Ogden.
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
Patchset:
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)
Attention is currently required from: Robert Ogden.
1 comment:
File third_party/pthreadpool/BUILD.gn:
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)
Attention is currently required from: Alex Gough, Honglin Yu, Junwei Fu, Robert Ogden.
23 comments:
Patchset:
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.
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:
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
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:
ComputeImplnit: 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 - 1this may be negative?
ack. I'll fix it by using base numerics as Alex suggested.
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_sizethis 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)
Attention is currently required from: Alex Gough, Honglin Yu, Junwei Fu, Robert Ogden.
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)
Attention is currently required from: Alex Gough, Honglin Yu, Junwei Fu, ningxin hu.
1 comment:
Patchset:
+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)
Attention is currently required from: Alex Gough, Honglin Yu, Junwei Fu, ningxin hu.
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)
Attention is currently required from: Alex Gough, Honglin Yu, Junwei Fu, Robert Ogden, ningxin hu.
2 comments:
Patchset:
+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_initializeWe 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)
Attention is currently required from: Alex Gough, Honglin Yu, Junwei Fu, Matthew Denton, Robert Ogden.
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)
Attention is currently required from: Alex Gough, Honglin Yu, Junwei Fu, Matthew Denton, Robert Ogden.
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)
Attention is currently required from: Alex Gough, Honglin Yu, Junwei Fu, Robert Ogden, ningxin hu.
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)
Attention is currently required from: Alex Gough, Honglin Yu, Junwei Fu, Matthew Denton, Robert Ogden.
8 comments:
Patchset:
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:
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?
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)
Attention is currently required from: Alex Gough, Honglin Yu, Junwei Fu, Matthew Denton, Robert Ogden.
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)
Attention is currently required from: Alex Gough, Junwei Fu, Matthew Denton, Robert Ogden, ningxin hu.
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)
Attention is currently required from: Alex Gough, Junwei Fu, Matthew Denton, ningxin hu.
1 comment:
Patchset:
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)
Attention is currently required from: Alex Gough, Junwei Fu, Matthew Denton, ningxin hu.
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)
Attention is currently required from: Junwei Fu, Matthew Denton, ningxin hu.
1 comment:
Patchset:
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)
Attention is currently required from: Alex Gough, Junwei Fu, Matthew Denton, ningxin hu.
1 comment:
Patchset:
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)
Attention is currently required from: Alex Gough, Junwei Fu, Matthew Denton.
1 comment:
Patchset:
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)
Attention is currently required from: Alex Gough, Honglin Yu, Junwei Fu, Matthew Denton.
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)
Attention is currently required from: Alex Gough, Junwei Fu, Matthew Denton, ningxin hu.
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)
Attention is currently required from: Alex Gough, Junwei Fu, ningxin hu.
1 comment:
File content/app/content_main_runner_impl.cc:
Patch Set #7, Line 414:
/cpu/cpuinfothird_party/tflite/src/third_party/cpuinfo/
To view, visit change 3684745. To unsubscribe, or for help writing mail filters, visit settings.
Matthew Denton (Gerrit)
File content/app/content_main_runner_impl.cc:
Patch Set #7, Line 414:
/cpu/cpuinfothird_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)
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)
Attention is currently required from: Honglin Yu, Junwei Fu.
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)
Attention is currently required from: Honglin Yu, Robert Ogden.
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?
File third_party/pthreadpool/BUILD.gn:
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)
Attention is currently required from: Honglin Yu, ningxin hu.
1 comment:
File third_party/pthreadpool/BUILD.gn:
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)
Attention is currently required from: Honglin Yu, ningxin hu.
Patch set 9:Code-Review +1
1 comment:
Patchset:
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)
Attention is currently required from: Honglin Yu.
2 comments:
Patchset:
Prototype the async methods and restrict the sync methods in dedicated worker according to the spec change [1]. Please take another look. Thanks.
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)
Attention is currently required from: Honglin Yu.
1 comment:
Patchset:
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)
Attention is currently required from: ningxin hu.
Patch set 12:Commit-Queue +1
1 comment:
Patchset:
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)
Attention is currently required from: Honglin Yu.
1 comment:
Patchset:
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)
Attention is currently required from: ningxin hu.
Patch set 13:Commit-Queue +1
1 comment:
Patchset:
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)
Attention is currently required from: ningxin hu.
1 comment:
Patchset:
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)
Attention is currently required from: Honglin Yu.
1 comment:
Patchset:
I can help send email to acco...@chromium.org if you like.
Yes, that would be extremely helpful, please help.
You must provide an email address and at least a brief explanation of why you'd like access.
For implementing WebNN and WebML in Chromium
It is helpful to provide a name and company affiliation (if any) as well.
Intel
And no problem at all.
Thanks much Honglin!
To view, visit change 3684745. To unsubscribe, or for help writing mail filters, visit settings.
Honglin Yu (Gerrit)
Attention is currently required from: ningxin hu.
1 comment:
Patchset:
> I can help send email to acco...@chromium.org if you like. […]
No problem. I have already sent the email and CC'd you, please check your inbox.
To view, visit change 3684745. To unsubscribe, or for help writing mail filters, visit settings.
ningxin hu (Gerrit)
Attention is currently required from: Honglin Yu.
1 comment:
Patchset:
No problem. I have already sent the email and CC'd you, please check your inbox.
received, thanks!
To view, visit change 3684745. To unsubscribe, or for help writing mail filters, visit settings.
Lisha Guo (Gerrit)
Attention is currently required from: Honglin Yu, ningxin hu.
3 comments:
Patchset:
Add some suggestions about validations.
File third_party/blink/renderer/modules/ml/webnn/ml_graph_builder.cc:
Patch Set #13, Line 187:
if (options->hasBias()) {According to the W3C webnn spec, the bias should be a 1-D tensor with the shape of [output_channels]
Patch Set #13, Line 251:
if (input_channels % options->groups() == 0 &&Exception still needs to be thrown if (input_channels % options->groups ！= 0).
To view, visit change 3684745. To unsubscribe, or for help writing mail filters, visit settings.
Honglin Yu (Gerrit)
Attention is currently required from: ningxin hu.
1 comment:
Patchset:
> No problem. I have already sent the email and CC'd you, please check your inbox. […]
No problem. Glad to help!
To view, visit change 3684745. To unsubscribe, or for help writing mail filters, visit settings.
ningxin hu (Gerrit)
Attention is currently required from: Lisha Guo.
1 comment:
Patchset:
Add some suggestions about validations.
Good catch, thanks Lisha. I'll address them in following patch set.
To view, visit change 3684745. To unsubscribe, or for help writing mail filters, visit settings.
ningxin hu (Gerrit)
Attention is currently required from: Honglin Yu.
1 comment:
Patchset:
Regarding to the failed try bots, I'll fix them by:
1. Only enable WebNN XNNPACK backend for Windows10 and Linux on X86 architecture because the XNNPACK lib fails to build for other platforms.
2. Update the WPT test expectations to reflect the new features.
To view, visit change 3684745. To unsubscribe, or for help writing mail filters, visit settings.
ningxin hu (Gerrit)
Attention is currently required from: Honglin Yu, Lisha Guo.
4 comments:
Patchset:
Regarding to the failed try bots, I'll fix them by: […]
Fixed in PS14.
Patchset:
PS14 tries to fix the trybots failures.
Honglin, if possible, please help trigger the trybots again to check the results. Thanks!
File third_party/blink/renderer/modules/ml/webnn/ml_graph_builder.cc:
Patch Set #13, Line 187:
if (options->hasBias()) {According to the W3C webnn spec, the bias should be a 1-D tensor with the shape of [output_channels]
Fixed in PS14, please take another look. Thanks.
Patch Set #13, Line 251:
if (input_channels % options->groups() == 0 &&Exception still needs to be thrown if (input_channels % options->groups ！= 0).
ditto.
To view, visit change 3684745. To unsubscribe, or for help writing mail filters, visit settings.
ningxin hu (Gerrit)
Patchset:
Honglin, it seems I am still not able to run the trybots. Could you please help trigger once more? Thanks!
To view, visit change 3684745. To unsubscribe, or for help writing mail filters, visit settings.
Honglin Yu (Gerrit)
Attention is currently required from: ningxin hu.
Patch set 15:Commit-Queue +1
1 comment:
Patchset:
Honglin, it seems I am still not able to run the trybots. […]
Sure. Just let me know whenever you want to give it a go.
To view, visit change 3684745. To unsubscribe, or for help writing mail filters, visit settings.
Lisha Guo (Gerrit)
Patchset:
LGTM.Thanks!
File third_party/blink/renderer/modules/ml/webnn/ml_graph_builder.cc:
Patch Set #13, Line 187:
if (options->hasBias()) {
Fixed in PS14, please take another look. Thanks.
Done
Patch Set #13, Line 251:
if (input_channels % options->groups() == 0 &&
ditto.
Done
To view, visit change 3684745. To unsubscribe, or for help writing mail filters, visit settings.
Lisha Guo (Gerrit)
Attention is currently required from: Honglin Yu, ningxin hu.
2 comments:
Patchset:
Tiny issue. please take a look. Thanks!
File third_party/blink/renderer/modules/ml/webnn/ml_graph_builder.cc:
Patch Set #15, Line 607:
if (!CalculatePaddingForAutoPad(options->autoPad().AsEnum(),Here the sequence of the parameters of `CalculatePaddingForAutoPad` is not align with the function declaration.
To view, visit change 3684745. To unsubscribe, or for help writing mail filters, visit settings.
ningxin hu (Gerrit)
Attention is currently required from: Honglin Yu, Lisha Guo.
2 comments:
Patchset:
Upload PS16 that fixes:
1. XNNPACK build issue on some trybots
2. Pool2d padding calculation issueHonglin, could you please help trigger the trybots? Thanks!
File third_party/blink/renderer/modules/ml/webnn/ml_graph_builder.cc:
Patch Set #15, Line 607:
if (!CalculatePaddingForAutoPad(options->autoPad().AsEnum(),
Here the sequence of the parameters of `CalculatePaddingForAutoPad` is not align with the function d […]
good catch, thanks Lisha! Fixed it in PS16. Please take another look.
To view, visit change 3684745. To unsubscribe, or for help writing mail filters, visit settings.
Honglin Yu (Gerrit)
Attention is currently required from: Lisha Guo, ningxin hu.
Patch set 16:Commit-Queue +1
1 comment:
Patchset:
Upload PS16 that fixes: […]
Sure.
To view, visit change 3684745. To unsubscribe, or for help writing mail filters, visit settings.
ningxin hu (Gerrit)
Attention is currently required from: Honglin Yu, Lisha Guo.
1 comment:
Patchset:
Thanks for the dry run, Honglin. It seems there is only one remaining failure on chromeos-amd64-generic-rel: https://ci.chromium.org/ui/p/chromium/builders/try/chromeos-amd64-generic-rel/1195830/overview
I am not quite sure what the issue is and how I can fix it. Honglin, do you have any insights?
To view, visit change 3684745. To unsubscribe, or for help writing mail filters, visit settings.
Honglin Yu (Gerrit)
Attention is currently required from: Lisha Guo, ningxin hu.
Patch set 16:Commit-Queue +1
1 comment:
Patchset:
Thanks for the dry run, Honglin. […]
it should be an infra failure. Let's try it again using the cq.
To view, visit change 3684745. To unsubscribe, or for help writing mail filters, visit settings.
ningxin hu (Gerrit)
Attention is currently required from: Alex Gough, Honglin Yu.
1 comment:
Patchset:
Thanks Honglin. The PS16 passed the trybots.
Honglin and Alex, could you please take another look? In particular, the base numeric usage for shape calculation and the implementation of the async call. Thanks!
To view, visit change 3684745. To unsubscribe, or for help writing mail filters, visit settings.
Honglin Yu (Gerrit)
Attention is currently required from: Alex Gough, ningxin hu.
1 comment:
Patchset:
Thanks Honglin. The PS16 passed the trybots. […]
No problem, Ningxin! I will take another look when I have bandwidth.
The opinions of Alex and other security reviewers should be more important here. And besides security, I am wondering whether the threading model is acceptable? For example, should we run the computation in the threadpool or main thread? I have been thinking about this but didn't find the answer yet.
To view, visit change 3684745. To unsubscribe, or for help writing mail filters, visit settings.
ningxin hu (Gerrit)
Attention is currently required from: Alex Gough, Honglin Yu.
1 comment:
Patchset:
No problem, Ningxin! I will take another look when I have bandwidth. […]
Thanks Honglin.
Regarding to the threading model, in the latest implementation, both graph computation and build run in the threadpool.
There are some details:
1. The async graph build/compute (MLGraphXnnpack::BuildImpl/ComputeImpl) post (worker_pool::PostTask) and run the actual build/compute task (MLGraphXnnpack::BuildOnBackgroundThread/ComputeOnBackgroundThread) in a worker thread.
2. The sync graph build/compute (MLGraphXnnpack::BuildSyncImpl/ComputeSyncImpl) run in the invoking thread. As the sync methods are only exposed to worker, the actual graph build/compute also run off the main thread.
3. XNNPACK has its own pthreadpool to run its operators computation parallelly. The current implementation configures the number of threads as half of the number of logical cores.What do you think about this design?
To view, visit change 3684745. To unsubscribe, or for help writing mail filters, visit settings.
Lisha Guo (Gerrit)
Attention is currently required from: Alex Gough, Honglin Yu, ningxin hu.
1 comment:
File third_party/blink/renderer/modules/ml/webnn/ml_graph_builder.cc:
Patch Set #15, Line 607:
if (!CalculatePaddingForAutoPad(options->autoPad().AsEnum(),
good catch, thanks Lisha! Fixed it in PS16. Please take another look.
Done
To view, visit change 3684745. To unsubscribe, or for help writing mail filters, visit settings.
Lisha Guo (Gerrit)
File third_party/blink/renderer/modules/ml/webnn/ml_graph_builder.cc:
Patch Set #16, Line 481:
output_shape[minus1_dim_idx] = input_size / capacity;I suggest the exception should be thrown if `input / capacity != 0`
To view, visit change 3684745. To unsubscribe, or for help writing mail filters, visit settings.
Alex Gough (Gerrit)
Attention is currently required from: Honglin Yu, ningxin hu.
2 comments:
Patchset:
thanks it's good to see use of checked calculations.
another useful effort (in a different CL) would be to think about how this will be fuzzed - and perhaps add a fuzzer & corpus that calls the graph building and execution apis
File content/app/content_main_runner_impl.cc:
Patch Set #16, Line 421:
if (xnn_initialize(NULL) != xnn_status_success) {It would obviously be good to avoid any pre-sandbox work if possible.
Does this need to happen here on Windows? Can it happen once the sandbox is turned on there? Can we do this work later on any other platforms or avoid doing it on all platforms?
Could cpuinfo or xnnpack be modified to set themselves up in a way that is friendly to sandboxes?
Can the xnn allocator be provided so that partition alloc is used, rather than say heapalloc?
Can xnn fail through unsupported cpus - if so what happens? (our current requirements are here: https://support.google.com/chrome/a/answer/7100626?hl=en)
To view, visit change 3684745. To unsubscribe, or for help writing mail filters, visit settings.
Honglin Yu (Gerrit)
Attention is currently required from: ningxin hu.
1 comment:
Patchset:
Thanks Honglin. […]
Thanks for the explanation, Ningxin! It makes sense to me. @skyostil: would you please help do an expert review on this? 😊 Thanks!
To view, visit change 3684745. To unsubscribe, or for help writing mail filters, visit settings.
ningxin hu (Gerrit)
Attention is currently required from: Honglin Yu, Lisha Guo, Sami Kyöstilä.
3 comments:
Patchset:
Fixed in PS14.
Done
Patchset:
PS17 fixes two issues:
1. need to validate the calculation of unknown dimension size for reshape
2. avoid add duplicated constants when sort the operatorsLisha and Junwei, please take a look. Thanks.
File third_party/blink/renderer/modules/ml/webnn/ml_graph_builder.cc:
Patch Set #16, Line 481:
output_shape[minus1_dim_idx] = input_size / capacity;I suggest the exception should be thrown if `input / capacity != 0`
Thanks Lisha. Fixed it in PS17. Please take a look.
To view, visit change 3684745. To unsubscribe, or for help writing mail filters, visit settings.
ningxin hu (Gerrit)
Attention is currently required from: Alex Gough, Honglin Yu, Lisha Guo, Sami Kyöstilä.
2 comments:
Patchset:
thanks it's good to see use of checked calculations. […]
That's a good idea. I'll follow up in a separate CL.
File content/app/content_main_runner_impl.cc:
Patch Set #16, Line 421:
if (xnn_initialize(NULL) != xnn_status_success) {
Does this need to happen here on Windows? Can it happen once the sandbox is turned on there? Can we do this work later on any other platforms or avoid doing it on all platforms?
No. On Windows, the xnnpack can be initialized after sandbox is turned on.
Could cpuinfo or xnnpack be modified to set themselves up in a way that is friendly to sandboxes?
This needs to be investigated.
So are you suggesting this CL just supports Windows? and support other platforms in following CL? This sounds good to me. I can remove the pre-sandbox initialization code for Linux. Please let me know.
Can the xnn allocator be provided so that partition alloc is used, rather than say heapalloc?
It looks like possible. I'll investigate and get you back.
Can xnn fail through unsupported cpus - if so what happens? (our current requirements are here: https://support.google.com/chrome/a/answer/7100626?hl=en)
I understand during the initialization, xnnpack will check whether the CPU is supported [1], otherwise it will return xnn_status_unsupported_hardware error. The following xnnpack calls will return xnn_status_uninitialized error. In that case, the WebNN call will throw DOMExcpetion correspondingly.
The xnnpack supported CPU architectures are listed at [2]. I suppose it meet the chrome system requirements.
[1]: https://github.com/google/XNNPACK/blob/master/src/init.c#L85
[2]: https://github.com/google/XNNPACK#supported-architectures
To view, visit change 3684745. To unsubscribe, or for help writing mail filters, visit settings.
Junwei (Gerrit)
Attention is currently required from: Alex Gough, Honglin Yu, Lisha Guo, Sami Kyöstilä, ningxin hu.
1 comment:
Patchset:
PS17 fixes two issues: […]
LGTM, thanks.
To view, visit change 3684745. To unsubscribe, or for help writing mail filters, visit settings.
Alex Gough (Gerrit)
Attention is currently required from: Honglin Yu, Sami Kyöstilä, ningxin hu.
1 comment:
File content/app/content_main_runner_impl.cc:
Patch Set #16, Line 421:
if (xnn_initialize(NULL) != xnn_status_success) {
> Does this need to happen here on Windows? Can it happen once the sandbox is turned on there? Can w […]
thanks - keep in the linux support but do move the windows initialization later if possible.
To view, visit change 3684745. To unsubscribe, or for help writing mail filters, visit settings.
Lisha Guo (Gerrit)
Attention is currently required from: Honglin Yu, Sami Kyöstilä, ningxin hu.
2 comments:
Patchset:
LGTM.Thanks!
File third_party/blink/renderer/modules/ml/webnn/ml_graph_builder.cc:
Patch Set #16, Line 481:
output_shape[minus1_dim_idx] = input_size / capacity;
Thanks Lisha. Fixed it in PS17. Please take a look.
Done
To view, visit change 3684745. To unsubscribe, or for help writing mail filters, visit settings.
ningxin hu (Gerrit)
Attention is currently required from: Alex Gough, Honglin Yu, Sami Kyöstilä.
1 comment:
File content/app/content_main_runner_impl.cc:
Patch Set #16, Line 421:
if (xnn_initialize(NULL) != xnn_status_success) {
thanks - keep in the linux support but do move the windows initialization later if possible.
I suppose the xnnpack initialization here is only for Linux, because this code is guarded by build flag check "BUILDFLAG(IS_LINUX) || BUILDFLAG(IS_CHROMEOS)" at line 367. Probably I could make it clear in the comment. WDYT?
The initialization on Windows happens later in "SharedXnnpackContext::Initialize" when MLGraphXnnpack is created.
Does this implementation align with your suggestion?
To view, visit change 3684745. To unsubscribe, or for help writing mail filters, visit settings.
Sami Kyöstilä (Gerrit)
Attention is currently required from: Alex Gough, Honglin Yu, ningxin hu.
5 comments:
Patchset:
Thanks for the patch! I've added a few comments about threading.
File third_party/blink/renderer/modules/ml/webnn/ml_graph_xnnpack.cc:
Patch Set #18, Line 43:
instance_This doesn't look thread safe. We might want to use `base::NoDestructor<SharedXnnPackContext>` or alternatively ` DEFINE_THREAD_SAFE_STATIC_LOCAL` -- unless it's important to tear this down after the computation.
Patch Set #18, Line 66:
pthreadpool_createCould we reuse base::ThreadPool here instead of spinning up a competing thread pool?
A lighter weight option would be base::PostJob if we're expecting extremely many small tasks to be posted.
I suggest annotating at least this method with `TRACE_EVENT("blink", "MLGraphXnnpack::InvokeRuntime")` to make it more obvious in traces since it's likely to use a lot of CPU time.
Patch Set #18, Line 1169:
xnn_invoke_runtimeJust curious: does the thread calling `xnn_invoke_runtime` also participate in the inference or is it just parked while the threadpool does the work? The former could improve performance a bit since the calling thread is already running.
To view, visit change 3684745. To unsubscribe, or for help writing mail filters, visit settings.
Alex Gough (Gerrit)
Attention is currently required from: Honglin Yu, ningxin hu.
1 comment:
File content/app/content_main_runner_impl.cc:
Patch Set #16, Line 421:
if (xnn_initialize(NULL) != xnn_status_success) {
I suppose the xnnpack initialization here is only for Linux, because this code is guarded by build f […]
ah you're right - so many buildflags!
To view, visit change 3684745. To unsubscribe, or for help writing mail filters, visit settings.
Rafael Cintron (Gerrit)
Attention is currently required from: Honglin Yu, Sami Kyöstilä, ningxin hu.
4 comments:
Patchset:
I have not finished reviewing this large change. Will finish next week.
File third_party/blink/renderer/modules/ml/webnn/ml_graph_builder.cc:
Vector<int32_t> dims_input0,
Vector<int32_t> dims_input1,Since dims_input0 and dims_input1 are passed by value, a copy is made of the vector on the stack. Since BroadcastShape only reads from the vectors and doesn't take ownership of them, you should pass these by const reference instead.
Patch Set #18, Line 245:
dividesNit: divides --> divide
File third_party/blink/renderer/modules/ml/webnn/ml_graph_xnnpack.cc:
Patch Set #18, Line 43:
instance_
This doesn't look thread safe. […]
+1 to not being thread safe.
The comments at the top of base/memory/singleton.h provide a good set of options for how to make a singleton that is only initialized once in a threadsafe manner.
However, even after initialization, the singleton needs to, itself, be threadsafe. Is that possible?
To view, visit change 3684745. To unsubscribe, or for help writing mail filters, visit settings.
ningxin hu (Gerrit)
Attention is currently required from: Honglin Yu, Rafael Cintron, Sami Kyöstilä.
7 comments:
Patchset:
Sami and Rafael, thanks for your review.
File third_party/blink/renderer/modules/ml/webnn/ml_graph_builder.cc:
Vector<int32_t> dims_input0,
Vector<int32_t> dims_input1,
Since dims_input0 and dims_input1 are passed by value, a copy is made of the vector on the stack. […]
You are right. I'll use const reference for dims_input0 and dims_input1.
Patch Set #18, Line 245:
dividesNit: divides --> divide
Ack
File third_party/blink/renderer/modules/ml/webnn/ml_graph_xnnpack.cc:
Patch Set #18, Line 43:
instance_
Yes, this is not thread safe. I'll fix that.
unless it's important to tear this down after the computation.
As the SharedXnnpackContext owns the pthreadpool, I suppose it would be good to tear it down if there are no MLGraphXnnpack objects use it. I understand singleton object would be created and live for the whole process lifetime. So I'd make it reference counted.
WDYT?
Patch Set #18, Line 66:
pthreadpool_createCould we reuse base::ThreadPool here instead of spinning up a competing thread pool?
I am afraid it would require a substantial change of xnnpack implementation because the pthreadpool is customized and coupled with xnnpack internally to run operators [1].
And the two thread pools may have different usages. I suppose base::ThreadPool is usually used for task background execution. However the pthreadpool, as its README says [2], provides similar functionality to "#pragma omp parallel for" which is for parallel computation. Is there any alternative one within Chromium?
[1]: https://source.chromium.org/chromium/chromium/src/+/main:third_party/xnnpack/src/src/operator-run.c;l=1337?q=operator-run.c
[2]: https://source.chromium.org/chromium/chromium/src/+/main:third_party/pthreadpool/src/README.md
I suggest annotating at least this method with `TRACE_EVENT("blink", "MLGraphXnnpack::InvokeRuntime" […]
Great suggestion. I'll add it here and probably also "MLGraphXnnpack::CreateRuntime" which will optimize the graph.
Patch Set #18, Line 1169:
xnn_invoke_runtime
Just curious: does the thread calling `xnn_invoke_runtime` also participate in the inference or is i […]
The calling thread would do computation as worker 0 [1].
To view, visit change 3684745. To unsubscribe, or for help writing mail filters, visit settings.
Sami Kyöstilä (Gerrit)
Attention is currently required from: Honglin Yu, Rafael Cintron, ningxin hu.
3 comments:
File third_party/blink/renderer/modules/ml/webnn/ml_graph_xnnpack.cc:
Patch Set #18, Line 43:
instance_
Yes, this is not thread safe. I'll fix that. […]
Agreed, tearing down the thread pool when not needed is probably a good idea if we can do it safely.
Patch Set #18, Line 66:
pthreadpool_create> Could we reuse base::ThreadPool here instead of spinning up a competing thread pool? […]
I see, I thought they might be quite tightly coupled. The closest analog in Chrome would be `base::PostJob`, and while from a quick skim `pthreadpool_parallelize` could be made to use it instead, it might not be worth the trouble at this point. How about we add a TODO here as reminder to consider `base::PostJob` in the future.
Btw, let's make this `std::max(1, base::SysInfo::NumberOfProcessors() / 2)` so that the pool size is predictable even if there's just one hardware thread available (e.g., when running in a virtualized container).
Patch Set #18, Line 1169:
xnn_invoke_runtime
The calling thread would do computation as worker 0 [1]. […]
Interesting, thanks for checking!
To view, visit change 3684745. To unsubscribe, or for help writing mail filters, visit settings.
ningxin hu (Gerrit)
Attention is currently required from: Alex Gough, Honglin Yu, Rafael Cintron, Sami Kyöstilä.
6 comments:
Patchset:
Upload PS 19 and 20 to address the following comments:
1. Use partition alloc to implement xnn allocator and constant data. (Alex)
2. Make the creation of SharedXnnpackContext thread safe. (Sami and Rafael)
3. Add TODO of using base::PostJob in the future (Sami)
4. Make thread number of pthreadpool predictable (Sami)
5. Use const reference for BroadcastShape (Rafael)
6. A typo (Rafael)Alex, Sami and Rafael, please take another look. Thanks!
File content/app/content_main_runner_impl.cc:
Patch Set #16, Line 421:
if (xnn_initialize(NULL) != xnn_status_success) {
Can the xnn allocator be provided so that partition alloc is used, rather than say heapalloc?
I tried to use buffer partition alloc to implement the xnn allocator for Windows in PS20. However, xnn allocator requires aligned allocation with XNN_ALLOCATION_ALIGNMENT which is platform dependent, e.g., 64 for X86/64 [1]. So I also proposed to change BufferPartition to allow aligned allocation. Please take a look. If it looks good, I'll implement it for Linux as well.
File third_party/blink/renderer/modules/ml/webnn/ml_graph_builder.cc:
Vector<int32_t> dims_input0,
Vector<int32_t> dims_input1,
You are right. I'll use const reference for dims_input0 and dims_input1.
Fixed in PS20. Please take another look. Thanks.
Patch Set #18, Line 245:
divides
Ack
Fixed in PS20.
File third_party/blink/renderer/modules/ml/webnn/ml_graph_xnnpack.cc:
Patch Set #18, Line 43:
instance_
Agreed, tearing down the thread pool when not needed is probably a good idea if we can do it safely.
In PS19, I tried to fir it by using DEFINE_THREAD_SAFE_STATIC_LOCAL to define a lock (SharedXnnpackContextLock) and use that lock to protect the instance_ creation. Please take a look.
Patch Set #18, Line 66:
pthreadpool_create
How about we add a TODO here as reminder to consider base::PostJob in the future.
sgtm. I added it in PS20.
let's make this std::max(1, base::SysInfo::NumberOfProcessors() / 2)
It makes sense. Thanks! Fixed in PS20.
Please take another look.
To view, visit change 3684745. To unsubscribe, or for help writing mail filters, visit settings.
Honglin Yu (Gerrit)
Attention is currently required from: Alex Gough, Rafael Cintron, Sami Kyöstilä, ningxin hu.
1 comment:
File third_party/blink/renderer/modules/ml/webnn/ml_graph_xnnpack.cc:
Patch Set #18, Line 43:
instance_
In PS19, I tried to fir it by using DEFINE_THREAD_SAFE_STATIC_LOCAL to define a lock (SharedXnnpackC […]
AFAIK, task posting is usually preferred over locks especially when it is not performance crucial. Maybe you could try to post the task to the kInternalDefault task runner (is it the right place?)? e.g.,
```
task_runner = execution_context->GetTaskRunner(kInternalDefault);
if (!task_runner->RunsTasksInCurrentSequence()) {
task_runner->PostTask(FUNC, ...);
}
```
To view, visit change 3684745. To unsubscribe, or for help writing mail filters, visit settings.
Sami Kyöstilä (Gerrit)
Attention is currently required from: Alex Gough, Honglin Yu, Rafael Cintron, ningxin hu.
5 comments:
File third_party/blink/renderer/modules/ml/webnn/ml_graph_xnnpack.cc:
Patch Set #18, Line 43:
instance_
AFAIK, task posting is usually preferred over locks especially when it is not performance crucial. […]
While posting tasks would be more straightforward, I guess the issue here is that SharedXnnpackContext can be concurrently accessed by multiple threads. With that constraint I don't think we can easily avoid at least some kind of locking around the shared instance.
File third_party/blink/renderer/modules/ml/webnn/ml_graph_xnnpack.cc:
Patch Set #20, Line 76:
bool Initialize() {If we did this initialization as a part of GetInstance(), could we avoid this second layer of locking? It seems like after the context is initialized, the thread pool can be retrieved without locking as long as each user keeps the context alive with a shared_refptr.
If we do this, then it's probably worth changing MLGraphXnnpack to only call GetInstance() when it's actually needed (e.g., in CreateRuntime).
SharedXnnpackContext() : initialized_(false) { instance_ = this; }
~SharedXnnpackContext() {We also need to hold the shared lock while setting and resetting the instance here.
Patch Set #20, Line 122:
instance_ = nullptr;For safety: DCHECK_EQ(this, instance_);
Patch Set #20, Line 133:
pthreadpool_Please default-initialize this to nullptr.
To view, visit change 3684745. To unsubscribe, or for help writing mail filters, visit settings.
Honglin Yu (Gerrit)
Attention is currently required from: Alex Gough, Rafael Cintron, Sami Kyöstilä, ningxin hu.
1 comment:
File third_party/blink/renderer/modules/ml/webnn/ml_graph_xnnpack.cc:
Patch Set #18, Line 43:
instance_
While posting tasks would be more straightforward, I guess the issue here is that SharedXnnpackConte […]
Yeah, locking is necessary. I may be wrong but I was thinking that normally it is preferred that locking is done inside the task runners and we just use its thread-safe interfaces[1] (for this case, maybe use things like `PostTaskAndReply`). But I agree that it will need much more boilerplate code and a bare lock is more clean here.
To view, visit change 3684745. To unsubscribe, or for help writing mail filters, visit settings.
ningxin hu (Gerrit)
Attention is currently required from: Honglin Yu, Sami Kyöstilä.
6 comments:
Patchset:
Hi Sami and Honglin, I tried to address your comments about locking in PS21. Please kindly take a look. Thanks!
File third_party/blink/renderer/modules/ml/webnn/ml_graph_xnnpack.cc:
Patch Set #18, Line 43:
instance_
Yeah, locking is necessary. […]
Thanks for the reference, Honglin. It is very helpful. Agreed with you and Sami about using lock to make it thread safe.
File third_party/blink/renderer/modules/ml/webnn/ml_graph_xnnpack.cc:
Patch Set #20, Line 76:
bool Initialize() {
Great suggestion. I tried to implement it in PS21. PTAL.
as long as each user keeps the context alive with a shared_refptr.
The scoped_refptr is used by MLGraphXnnpack to keep the context alive. Please let me know whether it looks right.
SharedXnnpackContext() : initialized_(false) { instance_ = this; }
~SharedXnnpackContext() {We also need to hold the shared lock while setting and resetting the instance here.
The constructor is only called by GetInstance() where the shared lock is held. So I only fixed the destructor by holding the shared lock while resetting the instance. Please take a look at PS21.
Patch Set #20, Line 122:
instance_ = nullptr;For safety: DCHECK_EQ(this, instance_);
Fixed in PS21.
Patch Set #20, Line 133:
pthreadpool_Please default-initialize this to nullptr.
Fixed in PS21
To view, visit change 3684745. To unsubscribe, or for help writing mail filters, visit settings.
Sami Kyöstilä (Gerrit)
Attention is currently required from: Honglin Yu, ningxin hu.
Patch set 21:Code-Review +1
3 comments:
Patchset:
Thanks, threading lgtm with a couple more suggestions.
File third_party/blink/renderer/modules/ml/webnn/ml_graph_xnnpack.cc:
Patch Set #21, Line 69:
nullptrLet's also delete the broken instance here so that further calls to GetInstance() won't return the half-initialized object.
Patch Set #21, Line 127:
instance_Can we put a GUARDED_BY(SharedXnnpackContextLock()) here?
To view, visit change 3684745. To unsubscribe, or for help writing mail filters, visit settings.
ningxin hu (Gerrit)
Attention is currently required from: Honglin Yu, Sami Kyöstilä.
2 comments:
File third_party/blink/renderer/modules/ml/webnn/ml_graph_xnnpack.cc:
Patch Set #21, Line 69:
nullptr
Let's also delete the broken instance here so that further calls to GetInstance() won't return the h […]
I suppose the instance (scoped_refptr) will be released when this function returns nullptr. Because there is no other reference at this point, the newly created SharedXnnpackContext will be deleted as well. When SharedXnnpackContext destructor is invoked, the instance_ will be reset to nullptr.
Do you mean I need to call instance.reset() to release it explicitly before function returns nullptr?
Patch Set #21, Line 127:
instance_Can we put a GUARDED_BY(SharedXnnpackContextLock()) here?
Sure. I'll fix that.
To view, visit change 3684745. To unsubscribe, or for help writing mail filters, visit settings.
ningxin hu (Gerrit)
Patchset:
PS22 rebases this CL to solve the merge conflicts.
Honglin, could you please help trigger the trybots to check if there are any issues after rebasing? Thanks!
File third_party/blink/renderer/modules/ml/webnn/ml_graph_xnnpack.cc:
Patch Set #21, Line 127:
instance_
Sure. I'll fix that.
Fixed in PS22.
To view, visit change 3684745. To unsubscribe, or for help writing mail filters, visit settings.
Honglin Yu (Gerrit)
Attention is currently required from: Sami Kyöstilä, ningxin hu.
Patch set 22:Commit-Queue +1
1 comment:
Patchset:
PS22 rebases this CL to solve the merge conflicts. […]
Sure. Not sure why there is such a delay. I will try to ping that email thread.
To view, visit change 3684745. To unsubscribe, or for help writing mail filters, visit settings.
ningxin hu (Gerrit)
Attention is currently required from: Honglin Yu, Sami Kyöstilä.
1 comment:
File third_party/blink/renderer/modules/ml/webnn/ml_graph_xnnpack.cc:
Patch Set #21, Line 69:
nullptrI suppose the instance (scoped_refptr) will be released when this function returns nullptr. […]
I realize there would be a deadlock issue if delete the broken instance inside GetInstance(), because the destructor will also hold the shared lock.
One possible solution would be to swap the sequence of instance initialization and creation. Only after the xnnpack initialization and pthreadpool creation succeed, the instance then is created. With that, there is no such a case of destructor will be called inside GetInstance(). WDYT?
I'll upload a new PS for review.
To view, visit change 3684745. To unsubscribe, or for help writing mail filters, visit settings.
ningxin hu (Gerrit)
File third_party/blink/renderer/modules/ml/webnn/ml_graph_xnnpack.cc:
Patch Set #21, Line 69:
nullptr
I realize there would be a deadlock issue if delete the broken instance inside GetInstance(), becaus […]
Fixed the deadlock issue in PS23. PTAL. Thanks!
To view, visit change 3684745. To unsubscribe, or for help writing mail filters, visit settings.
Sami Kyöstilä (Gerrit)
Attention is currently required from: Honglin Yu, ningxin hu.
Patch set 23:Code-Review +1
2 comments:
File third_party/blink/renderer/modules/ml/webnn/ml_graph_xnnpack.cc:
Patch Set #20, Line 122:
instance_ = nullptr;Fixed in PS21.
(Please don't forget to mark resolved comments as resolved so it's easier to see what's still outstanding.)
File third_party/blink/renderer/modules/ml/webnn/ml_graph_xnnpack.cc:
Patch Set #21, Line 69:
nullptr
Fixed the deadlock issue in PS23. PTAL. […]
Thanks, this should work now that there's no way to end up with a half-initialized instance and the destructor also won't run while we're holding the lock.
To view, visit change 3684745. To unsubscribe, or for help writing mail filters, visit settings.
Junwei Fu (Gerrit)
Attention is currently required from: Honglin Yu, ningxin hu.
1 comment:
File third_party/blink/renderer/modules/ml/webnn/ml_graph_xnnpack.cc:
Patch Set #18, Line 1126:
if (array_buffer_view->byteLength() < iter->value.byte_length) {How about moving the validation of input / output info to base class MLGraph which can be shared by GPU backend?
To view, visit change 3684745. To unsubscribe, or for help writing mail filters, visit settings.
ningxin hu (Gerrit)
Attention is currently required from: Honglin Yu, Rafael Cintron, Sami Kyöstilä.
10 comments:
Patchset:
Thanks for the explanation, Ningxin! It makes sense to me. […]
Threading model was reviewed by Sami. Mark it resolved.
File third_party/blink/renderer/modules/ml/webnn/ml_graph_builder.cc:
Vector<int32_t> dims_input0,
Vector<int32_t> dims_input1,
Fixed in PS20. Please take another look. Thanks.
Done
Patch Set #18, Line 245:
divides
Fixed in PS20.
Done
File third_party/blink/renderer/modules/ml/webnn/ml_graph_xnnpack.cc:
Patch Set #18, Line 43:
instance_Thanks for the reference, Honglin. It is very helpful. […]
It is now protected by the shared lock. Mark it resolved.
Patch Set #18, Line 66:
pthreadpool_create
> How about we add a TODO here as reminder to consider base::PostJob in the future. […]
Done
Great suggestion. […]
Done
File third_party/blink/renderer/modules/ml/webnn/ml_graph_xnnpack.cc:
Patch Set #20, Line 76:
bool Initialize() {Great suggestion. I tried to implement it in PS21. PTAL. […]
Done
SharedXnnpackContext() : initialized_(false) { instance_ = this; }
~SharedXnnpackContext() {
The constructor is only called by GetInstance() where the shared lock is held. […]
Done
Patch Set #20, Line 122:
instance_ = nullptr;
(Please don't forget to mark resolved comments as resolved so it's easier to see what's still outsta […]
sure, mark it resolved.
File third_party/blink/renderer/modules/ml/webnn/ml_graph_xnnpack.cc:
Patch Set #21, Line 69:
nullptr
Thanks, this should work now that there's no way to end up with a half-initialized instance and the […]
Done
To view, visit change 3684745. To unsubscribe, or for help writing mail filters, visit settings.
ningxin hu (Gerrit)
Attention is currently required from: Honglin Yu, Rafael Cintron, Sami Kyöstilä.
2 comments:
File third_party/blink/renderer/modules/ml/webnn/ml_graph_xnnpack.cc:
Patch Set #20, Line 133:
pthreadpool_Fixed in PS21
Done
File third_party/blink/renderer/modules/ml/webnn/ml_graph_xnnpack.cc:
Patch Set #21, Line 127:
instance_Fixed in PS22.
Done
ningxin hu (Gerrit)
Attention is currently required from: Sami Kyöstilä.
1 comment:
Patchset:
Sami, I marked the resolved comments as resolved. Please let me know if there are anything I missed. Thanks.
To view, visit change 3684745. To unsubscribe, or for help writing mail filters, visit settings.
ningxin hu (Gerrit)
Attention is currently required from: Alex Gough, Sami Kyöstilä.
2 comments:
Patchset:
Hi Alex, I uploaded PS24 that only initializes cpuinfo pre-sandbox on Linux and initializes xnnpack with buffer partition based xnn allocator after sandbox turned on for both Windows and Linux.
Please take another look. Thanks!
File content/app/content_main_runner_impl.cc:
Patch Set #16, Line 421:
if (xnn_initialize(NULL) != xnn_status_success) {
> Can the xnn allocator be provided so that partition alloc is used, rather than say heapalloc? […]
cpuinfo only needs to be initialized once [1], later invocation, e.g., by xnnpack_initialize, will reuse the initialized cpuinfo.
So in PS24, I only keep cpufino_initialize() here in pre-sandbox to access /proc/cpuinfo on Linux. The xnnpack_initialize() will be invoked by SharedXnnpackContext creation later after sandbox turned on. With that, now Windows and Linux implementations share the same xnnpack initialize/uninitialize code and both support the buffer partition based xnn allocator.
To view, visit change 3684745. To unsubscribe, or for help writing mail filters, visit settings.
Michael Crouse (Gerrit)
Attention is currently required from: Alex Gough, Sami Kyöstilä, ningxin hu.
Patch set 24:Code-Review +1
1 comment:
Patchset:
Just adding myself to keep track while robertogden@ is OOO.
To view, visit change 3684745. To unsubscribe, or for help writing mail filters, visit settings.