[Examples] Add OpenVINO benchmarking examples #6
Conversation
dimakuv
left a comment
dimakuv
left a comment
There was a problem hiding this comment.
Reviewed 5 of 5 files at r1, all commit messages.
Reviewable status: all files reviewed, 32 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @sahason)
a discussion (no related file):
Please rebase this example to the latest master. Also please make sure it still builds and runs correctly with the latest (v1.0) release of Gramine.
a discussion (no related file):
After a rebase, please replace all Graphene with Gramine.
-- commits, line 2 at r1:
No need for [Examples] prefix -- we are already in the Examples repo.
openvino/benchmark_app.manifest.template, line 7 at r1 (raw file):
loader.log_level = "{{ log_level }}" loader.env.LD_LIBRARY_PATH = "/lib:{{ openvino_dir }}/deployment_tools/inference_engine/external/tbb/lib:{{ openvino_dir }}/deployment_tools/inference_engine/lib/intel64:{{ arch_libdir }}:{{ openvino_dir }}/opencv/lib:{{ openvino_dir }}/deployment_tools/ngraph/lib:/usr/{{ arch_libdir }}"
Why do you have {{ arch_libdir }} and /usr/{{ arch_libdir }} separated by several OpenVINO paths? I mean, this looks weird; is there a particular reason?
If there is no reason for this, then I would prefer to have both these directories at the very end (just like in all other examples).
openvino/benchmark_app.manifest.template, line 9 at r1 (raw file):
loader.env.LD_LIBRARY_PATH = "/lib:{{ openvino_dir }}/deployment_tools/inference_engine/external/tbb/lib:{{ openvino_dir }}/deployment_tools/inference_engine/lib/intel64:{{ arch_libdir }}:{{ openvino_dir }}/opencv/lib:{{ openvino_dir }}/deployment_tools/ngraph/lib:/usr/{{ arch_libdir }}" loader.insecure__use_cmdline_argv = true
Could you move this line to other lines with true/false options? Just for readability.
openvino/benchmark_app.manifest.template, line 35 at r1 (raw file):
fs.mount.samples.uri = "file:{{ inference_engine_cpp_samples_build }}" sgx.nonpie_binary = true
Is this really needed? Pretty sure this line can be removed.
openvino/benchmark_app.manifest.template, line 47 at r1 (raw file):
"file:{{ openvino_dir }}/deployment_tools/inference_engine/lib/intel64/", "file:{{ openvino_dir }}/deployment_tools/inference_engine/external/tbb/lib/", "file:{{ inference_engine_cpp_samples_build }}/intel64/Release/lib/libformat_reader.so",
Can you move this line after all openvino_dir entries? Otherwise it looks weird: you have openvino_dir entries above and openvino_dir entries below.
openvino/benchmark_app.manifest.template, line 47 at r1 (raw file):
"file:{{ openvino_dir }}/deployment_tools/inference_engine/lib/intel64/", "file:{{ openvino_dir }}/deployment_tools/inference_engine/external/tbb/lib/", "file:{{ inference_engine_cpp_samples_build }}/intel64/Release/lib/libformat_reader.so",
Why not simply "file:{{ inference_engine_cpp_samples_build }}/intel64/Release/lib"? This way you'll cover all libraries under this path, which is less future-prone.
openvino/benchmark_app.manifest.template, line 49 at r1 (raw file):
"file:{{ inference_engine_cpp_samples_build }}/intel64/Release/lib/libformat_reader.so", "file:{{ openvino_dir }}/opencv/lib/", "file:{{ openvino_dir }}/deployment_tools/ngraph/lib/libngraph.so",
Why not simply "file:{{ openvino_dir }}/deployment_tools/ngraph/lib"? This way you'll cover all libraries under this path, which is less future-prone.
openvino/benchmark_app.manifest.template, line 59 at r1 (raw file):
"file:/etc/hosts", "file:/etc/group", "file:/etc/passwd",
Please sort these filenames.
openvino/Makefile, line 1 at r1 (raw file):
SHELL := /bin/bash #Use bash syntax to activate virtual environment and to download models
#Use -> # use
openvino/Makefile, line 9 at r1 (raw file):
ARCH_LIBDIR ?= /lib/$(shell $(CC) -dumpmachine) INFERENCE_ENGINE_CPP_SAMPLES_BUILD ?= $(THIS_DIR)inference_engine_cpp_samples_build
You have double-space here
openvino/Makefile, line 79 at r1 (raw file):
-Darch_libdir=$(ARCH_LIBDIR) \ -Dopenvino_dir=$(abspath $(OPENVINO_DIR)) \ -Dinference_engine_cpp_samples_build=$(abspath $(INFERENCE_ENGINE_CPP_SAMPLES_BUILD)) \
Please add another tab to make it explicit these are continuation lines of the graphene-manifest command
openvino/Makefile, line 87 at r1 (raw file):
graphene-sgx-sign \ -key $(SGX_SIGNER_KEY) \ -manifest $< -output $@
Please add another tab to make it explicit these are continuation lines of the graphene-sgx-sign command.
Also, we don't allow -argument anymore. It always has to be -- (double dash), as is classic in Linux.
openvino/Makefile, line 96 at r1 (raw file):
benchmark_app: $(OPENVINO_DIR) mkdir -p $(INFERENCE_ENGINE_CPP_SAMPLES_BUILD) cd $(INFERENCE_ENGINE_CPP_SAMPLES_BUILD) && cmake -DCMAKE_BUILD_TYPE=Release $(OPENVINO_DIR)/inference_engine/samples/cpp && make
Can you split it into three lines? Hard to read
openvino/README.md, line 8 at r1 (raw file):
learning inference performance. We test only the CPU backend (i.e., no GPU or FPGA). **NOTE**: the models require ~3GB of disk space.
This should be moved under a new section ## Notes
openvino/README.md, line 10 at r1 (raw file):
**NOTE**: the models require ~3GB of disk space. ## Tips for better performance
Actually, why is this a separate section? Why not move it under ## Performance considerations?
openvino/README.md, line 34 at r1 (raw file):
## Supported models for Graphene-SGX The following models have been enabled and tested with Graphene-SGX.
: instead of .
openvino/README.md, line 41 at r1 (raw file):
- bert-large-uncased-whole-word-masking-squad-int8-0001(INT8) - brain-tumor-segmentation-0001(FP16/FP32) - brain-tumor-segmentation-0002(FP16/FP32)
Please add a space before ( in all these lines
openvino/README.md, line 45 at r1 (raw file):
## Preparing the source 1. `cd $(GRAPHENE_DIR)/Examples/openvino_benchmark`
What's the point in this step? The user is already in this directory. Just remove this step.
openvino/README.md, line 49 at r1 (raw file):
`source /opt/intel/openvino_2021/bin/setupvars.sh` or you can permanently set it by appending `source /opt/intel/openvino_2021/bin/setupvars.sh` to `~/.bashrc`. For regular users run `source /home/<USER>/intel/openvino_2021/bin/setupvars.sh`.
This step is hard to read. Can you separate it into three sub-steps (one for root user, one for root user and set permanently, and one for regular user)?
openvino/README.md, line 53 at r1 (raw file):
**NOTE**: After setting up OpenVINO environment variables if you want to build Graphene after cleaning you need to unset LD_LIBRARY_PATH. Please make sure to set up OpenVINO environment
LD_LIBRARY_PATH must be in backticks
openvino/README.md, line 54 at r1 (raw file):
**NOTE**: After setting up OpenVINO environment variables if you want to build Graphene after cleaning you need to unset LD_LIBRARY_PATH. Please make sure to set up OpenVINO environment variables after building Graphene again.
This should be moved under a new section ## Notes
openvino/README.md, line 58 at r1 (raw file):
## Running the benchmark Performance benchmark on Xeon servers (Silver/Gold/Platinum) must be launched with increased number
Why on Xeon servers? What is so special about them? Can we just remove this detail?
openvino/README.md, line 63 at r1 (raw file):
performance. **NOTE**: To get `number of physical cores`, do `lscpu | grep 'Core(s) per socket'`.
Move this to the new section ## Notes, see my comment below.
openvino/README.md, line 75 at r1 (raw file):
-m model/<public | intel>/<model_dir>/<INT8 | FP16 | FP32>/<model_xml_file> \ -d CPU -b 1 -t 20 \ -nstreams OPTIMAL_VALUE -nthreads OPTIMAL_VALUE -nireq OPTIMAL_VALUE
Please add four spaces for indentation, to make it clear that these are continuation lines of the same command
openvino/README.md, line 87 at r1 (raw file):
with `./benchmark_app` in the above command. **NOTE 1**: Option `i <image files>` is optional. A user may use this option as required.
Why are these notes under #### Native? Please create a new section ## Notes for running the benchmark and move all these notes under there, as a bullet list. Also, please remove NOTE 1 from another sub-section.
Basically, I suggest the following layout:
## Running the benchmark
### Throughput runs
<no need for #### Graphene-SGX>
...
### Latency runs
<no need for #### Graphene-SGX>
...
## Notes
## Performance considerations
openvino/README.md, line 109 at r1 (raw file):
graphene-sgx benchmark_app -i <image files> \ -m model/<public | intel>/<model_dir>/<INT8 | FP16 | FP32>/<model_xml_file> \ -d CPU -b 1 -t 20 -api sync
Please add four spaces for indentation, to make it clear that these are continuation lines of the same command
openvino/README.md, line 117 at r1 (raw file):
with `./benchmark_app` in the above command. **NOTE**: Option `-i <image files>` is optional. A user may use this option as required.
Please remove this one, since we'll have a common ## Notes section now.
openvino/README.md, line 120 at r1 (raw file):
## Performance considerations
Actually, this section looks a bit ugly. Maybe use sub-sections instead of bullet list? Like this:
## Performance considerations
### Manifest options for performance
... preheat
... check invalid pointers
### Memory allocator libraries
#### TCMalloc
#### mimalloc
openvino/README.md, line 136 at r1 (raw file):
- `loader.env.LD_PRELOAD = "/usr/lib/x86_64-linux-gnu/libtcmalloc.so.4"` - `sgx.trusted_files.libtcmalloc = "file:/usr/lib/x86_64-linux-gnu/libtcmalloc.so.4"` - `sgx.trusted_files.libunwind = "file:/usr/lib/x86_64-linux-gnu/libunwind.so.8"`
The syntax sgx.trusted_files.xxx is deprecated. Please use something like sgx.trusted_files = [ ..., "file:/usr/lib/x86_64-linux-gnu/libunwind.so.8"]
openvino/README.md, line 145 at r1 (raw file):
- `fs.mount.usr_local.uri = "file:/usr/local"` - `loader.env.LD_PRELOAD = "/usr/local/lib/mimalloc-1.7/libmimalloc.so.1.7"` - `sgx.trusted_files.libmimalloc = "file:/usr/local/lib/mimalloc-1.7/libmimalloc.so.1.7"`
ditto
sahason
force-pushed
the
sahason/ov_benchmark
branch
from
fa28a74 to
1119dee
Compare
sahason
left a comment
sahason
left a comment
There was a problem hiding this comment.
Reviewable status: 2 of 5 files reviewed, 32 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @dimakuv)
a discussion (no related file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
After a rebase, please replace all
GraphenewithGramine.
Done.
a discussion (no related file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Please rebase this example to the latest master. Also please make sure it still builds and runs correctly with the latest (v1.0) release of Gramine.
Done.
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
No need for
[Examples]prefix -- we are already in the Examples repo.
Done.
openvino/benchmark_app.manifest.template, line 7 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Why do you have
{{ arch_libdir }}and/usr/{{ arch_libdir }}separated by several OpenVINO paths? I mean, this looks weird; is there a particular reason?If there is no reason for this, then I would prefer to have both these directories at the very end (just like in all other examples).
Done.
openvino/benchmark_app.manifest.template, line 9 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Could you move this line to other lines with
true/falseoptions? Just for readability.
Done.
openvino/benchmark_app.manifest.template, line 35 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Is this really needed? Pretty sure this line can be removed.
Done.
openvino/benchmark_app.manifest.template, line 47 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Can you move this line after all
openvino_direntries? Otherwise it looks weird: you haveopenvino_direntries above andopenvino_direntries below.
Done.
openvino/benchmark_app.manifest.template, line 47 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Why not simply
"file:{{ inference_engine_cpp_samples_build }}/intel64/Release/lib"? This way you'll cover all libraries under this path, which is less future-prone.
Done.
openvino/benchmark_app.manifest.template, line 49 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Why not simply
"file:{{ openvino_dir }}/deployment_tools/ngraph/lib"? This way you'll cover all libraries under this path, which is less future-prone.
Done.
openvino/benchmark_app.manifest.template, line 59 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Please sort these filenames.
Done.
openvino/Makefile, line 1 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
#Use-># use
Done.
openvino/Makefile, line 9 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
You have double-space here
Done.
openvino/Makefile, line 79 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Please add another tab to make it explicit these are continuation lines of the
graphene-manifestcommand
Done.
openvino/Makefile, line 87 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Please add another tab to make it explicit these are continuation lines of the
graphene-sgx-signcommand.Also, we don't allow
-argumentanymore. It always has to be--(double dash), as is classic in Linux.
Done.
openvino/Makefile, line 96 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Can you split it into three lines? Hard to read
Done.
openvino/README.md, line 8 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
This should be moved under a new section
## Notes
Done.
openvino/README.md, line 10 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Actually, why is this a separate section? Why not move it under
## Performance considerations?
Done.
openvino/README.md, line 34 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
:instead of.
Done.
openvino/README.md, line 41 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Please add a space before
(in all these lines
Done.
openvino/README.md, line 45 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
What's the point in this step? The user is already in this directory. Just remove this step.
Done.
openvino/README.md, line 49 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
This step is hard to read. Can you separate it into three sub-steps (one for root user, one for root user and set permanently, and one for regular user)?
Done.
openvino/README.md, line 53 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
LD_LIBRARY_PATHmust be in backticks
Done.
openvino/README.md, line 54 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
This should be moved under a new section
## Notes
Done.
openvino/README.md, line 58 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Why on
Xeon servers? What is so special about them? Can we just remove this detail?
Done.
openvino/README.md, line 63 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Move this to the new section
## Notes, see my comment below.
Done.
openvino/README.md, line 75 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Please add four spaces for indentation, to make it clear that these are continuation lines of the same command
Done.
openvino/README.md, line 87 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Why are these notes under
#### Native? Please create a new section## Notes for running the benchmarkand move all these notes under there, as a bullet list. Also, please removeNOTE 1from another sub-section.Basically, I suggest the following layout:
## Running the benchmark ### Throughput runs <no need for #### Graphene-SGX> ... ### Latency runs <no need for #### Graphene-SGX> ... ## Notes ## Performance considerations
Done. Keeping #### Graphene-SGX and #### Graphene-SGX to differentiate between native and Graphene
openvino/README.md, line 109 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Please add four spaces for indentation, to make it clear that these are continuation lines of the same command
Done.
openvino/README.md, line 117 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Please remove this one, since we'll have a common
## Notessection now.
Done.
openvino/README.md, line 120 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Actually, this section looks a bit ugly. Maybe use sub-sections instead of bullet list? Like this:
## Performance considerations ### Manifest options for performance ... preheat ... check invalid pointers ### Memory allocator libraries #### TCMalloc #### mimalloc
Done.
openvino/README.md, line 136 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
The syntax
sgx.trusted_files.xxxis deprecated. Please use something likesgx.trusted_files = [ ..., "file:/usr/lib/x86_64-linux-gnu/libunwind.so.8"]
Done.
openvino/README.md, line 145 at r1 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
ditto
Done.
dimakuv
left a comment
dimakuv
left a comment
There was a problem hiding this comment.
Reviewed 3 of 3 files at r2, all commit messages.
Reviewable status: all files reviewed, 23 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @sahason)
a discussion (no related file):
You should add more "git ignore" files/dirs to .gitignore. In particular, you should add at least these:
/inference_engine_cpp_samples_build/benchmark_app/output
Basically, you need to add all files/dirs that are created during OpenVINO build/run.
-- commits, line 3 at r2:
examples -> example
-- commits, line 4 at r2:
We'll need to add a commit body message like this:
This commit replaces the old OpenVINO `object_detection_sample_ssd` example.
Now the OpenVINO example may be used for benchmarking purposes.
We'll do it during final rebase.
openvino/benchmark_app.manifest.template, line 1 at r2 (raw file):
# OpenVINO (benchmark_app) manifest example
Could you remove this comment (and the following empty line)? At some point we decided to remove such useless comments (the user is already in this directory, so the user knows what this manifest file is about).
openvino/benchmark_app.manifest.template, line 34 at r2 (raw file):
sgx.enclave_size = "32G" sgx.thread_num = 196
Could you add an empty line after this, so there is a clear distinction between SGX-numbers options and true/false options.
openvino/benchmark_app.manifest.template, line 49 at r2 (raw file):
"file:{{ inference_engine_cpp_samples_build }}/intel64/Release/lib/", "file:benchmark_app", "file:./model/"
Could you replace ./model/ with simply model/
Also, please keep the comma at the end of the line (like it was before) -- this is allowed in TOML files, and it will be easier to add more lines to this list in the future.
openvino/benchmark_app.manifest.template, line 58 at r2 (raw file):
"file:/etc/nsswitch.conf", "file:/etc/passwd", "file:output.txt"
please keep the comma at the end of the line (like it was before) -- this is allowed in TOML files, and it will be easier to add more lines to this list in the future.
openvino/README.md, line 27 at r2 (raw file):
- bert-large-uncased-whole-word-masking-squad-int8-0001 (INT8) - brain-tumor-segmentation-0001 (FP16/FP32) - brain-tumor-segmentation-0002 (FP16/FP32)
There seems to be no logic in the ordering of these models. Can we sort them alphabetically?
openvino/README.md, line 89 at r2 (raw file):
- To get `number of physical cores`, do `lscpu | grep 'Core(s) per socket'`. - Option `i <image files>` is optional. A user may use this option as required. - Please tune batch size to get best performance in your system.
the batch size, the best performance
in your system -> on your system
openvino/README.md, line 93 at r2 (raw file):
the models these are stored in `model/public` directory. - Based on the precision for bert-large and brain-tumor-segmentation models the enclave size must be set to 64/128 GB for capturing throughput.
I don't understand this note. What does it mean "based on precision"? Could you expand on this -- what kinds of precision need bigger enclave sizes? And why is it only about throughput, what about latency?
openvino/README.md, line 96 at r2 (raw file):
- In multi-socket systems for bert-large-uncased-whole-word-masking-squad-0001 and brain-tumor-segmentation-0001 FP32/FP16 models please expand memory nodes usage with `numactl --membind` if memory allocation fails for capturing throughput.
What does it mean "expand memory nodes usage"? Do you mean that the user must membind to more than one NUMA node? Please expand.
openvino/README.md, line 100 at r2 (raw file):
## Performance considerations ### Scale the CPU frequency
Let's rename to CPU frequency scaling
openvino/README.md, line 115 at r2 (raw file):
- Preheat manifest option pre-faults the enclave memory and moves the performance penalty to gramine-sgx startup (before the workload starts executing). To use preheat option, add
use preheat option -> use the preheat option
openvino/README.md, line 116 at r2 (raw file):
- Preheat manifest option pre-faults the enclave memory and moves the performance penalty to gramine-sgx startup (before the workload starts executing). To use preheat option, add `sgx.preheat_enclave = true` to the manifest template.
You already have this option in the manifest template. Maybe rephrase to
To use the preheat option, make sure that `sgx.preheat_enclave = true` is added to
the manifest template.
openvino/README.md, line 120 at r2 (raw file):
invalid pointers (typical case) can help improve performance. To use this option, add `libos.check_invalid_pointers = false` to the manifest template.
You already have this option in the manifest template. Please rephrase similar to as above.
openvino/README.md, line 130 at r2 (raw file):
#### TCMalloc Please update the binary location and name if different from default.
Please put this sentence in braces (( and ))
openvino/README.md, line 131 at r2 (raw file):
Please update the binary location and name if different from default. - Install tcmalloc : `sudo apt-get install google-perftools`
Remove one space before :
openvino/README.md, line 132 at r2 (raw file):
Please update the binary location and name if different from default. - Install tcmalloc : `sudo apt-get install google-perftools` - Add these in the manifest template:
Let's replace with Modify the manifest file:
openvino/README.md, line 133 at r2 (raw file):
- Install tcmalloc : `sudo apt-get install google-perftools` - Add these in the manifest template: - `loader.env.LD_PRELOAD = "/usr/lib/x86_64-linux-gnu/libtcmalloc.so.4"`
Let's prepend with "add", like this:
Add `loader.env.LD_PRELOAD = ...`
openvino/README.md, line 141 at r2 (raw file):
#### mimalloc Please update the binary location and name if different from default.
Please put this sentence in braces (( and ))
openvino/README.md, line 143 at r2 (raw file):
Please update the binary location and name if different from default. - Install mimalloc using the steps from https://github.com/microsoft/mimalloc - Add these in the manifest template:
Let's replace with Modify the manifest file:
openvino/README.md, line 144 at r2 (raw file):
- Install mimalloc using the steps from https://github.com/microsoft/mimalloc - Add these in the manifest template: - `fs.mount.usr_local.type = "chroot"`
Could you add a sub-list for three fs.mount.usr_local lines? Just like you did for sgx.trusted_files. This will be visually more clear.
So it will be something like this:
- Add the `/usr/local` FS mount point:
- `fs.mount.usr_local.type = "chroot"`
- `fs.mount.usr_local.path = "/usr/local"`
- `fs.mount.usr_local.uri = "file:/usr/local"`
openvino/README.md, line 147 at r2 (raw file):
- `fs.mount.usr_local.path = "/usr/local"` - `fs.mount.usr_local.uri = "file:/usr/local"` - `loader.env.LD_PRELOAD = "/usr/local/lib/mimalloc-1.7/libmimalloc.so.1.7"`
Let's prepend with "add", like this:
Add `loader.env.LD_PRELOAD = ...`
ef8c052 to
71b5178
Compare
sahason
left a comment
sahason
left a comment
There was a problem hiding this comment.
Reviewable status: 3 of 6 files reviewed, 23 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @dimakuv)
a discussion (no related file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
You should add more "git ignore" files/dirs to
.gitignore. In particular, you should add at least these:
/inference_engine_cpp_samples_build/benchmark_app/outputBasically, you need to add all files/dirs that are created during OpenVINO build/run.
Done.
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
examples->example
Done.
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
We'll need to add a commit body message like this:
This commit replaces the old OpenVINO `object_detection_sample_ssd` example. Now the OpenVINO example may be used for benchmarking purposes.We'll do it during final rebase.
OK.
openvino/benchmark_app.manifest.template, line 1 at r2 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Could you remove this comment (and the following empty line)? At some point we decided to remove such useless comments (the user is already in this directory, so the user knows what this manifest file is about).
Done.
openvino/benchmark_app.manifest.template, line 34 at r2 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Could you add an empty line after this, so there is a clear distinction between SGX-numbers options and
true/falseoptions.
Done.
openvino/benchmark_app.manifest.template, line 49 at r2 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Could you replace
./model/with simplymodel/Also, please keep the comma at the end of the line (like it was before) -- this is allowed in TOML files, and it will be easier to add more lines to this list in the future.
Done.
openvino/benchmark_app.manifest.template, line 58 at r2 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
please keep the comma at the end of the line (like it was before) -- this is allowed in TOML files, and it will be easier to add more lines to this list in the future.
Done.
openvino/README.md, line 27 at r2 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
There seems to be no logic in the ordering of these models. Can we sort them alphabetically?
Done.
openvino/README.md, line 89 at r2 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
the batch size,the best performance
in your system->on your system
Done.
openvino/README.md, line 93 at r2 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
I don't understand this note. What does it mean "based on precision"? Could you expand on this -- what kinds of precision need bigger enclave sizes? And why is it only about throughput, what about latency?
I will remove precision. It mostly depends on the model size of different models. For bert-large it requires 64 GB and for brain-tumor-segmentation-0001 and 0002 it requires 128 GB memory.
No for latency it does not change based on the model. Options -nireq, -nstreams and -nthreads are set as number of physical cores * 2 for throughput capturing whereas these options are set as 1 by default for latency capturing hence for latency 32 GB enclave size is enough for all the models.
openvino/README.md, line 96 at r2 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
What does it mean "expand memory nodes usage"? Do you mean that the user must
membindto more than one NUMA node? Please expand.
Yes.
openvino/README.md, line 100 at r2 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Let's rename to
CPU frequency scaling
Done.
openvino/README.md, line 115 at r2 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
use preheat option->use the preheat option
Done.
openvino/README.md, line 116 at r2 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
You already have this option in the manifest template. Maybe rephrase to
To use the preheat option, make sure that `sgx.preheat_enclave = true` is added to the manifest template.
Done.
openvino/README.md, line 120 at r2 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
You already have this option in the manifest template. Please rephrase similar to as above.
Done.
openvino/README.md, line 130 at r2 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Please put this sentence in braces (
(and))
Done.
openvino/README.md, line 131 at r2 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Remove one space before
:
Done.
openvino/README.md, line 132 at r2 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Let's replace with
Modify the manifest file:
Done.
openvino/README.md, line 133 at r2 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Let's prepend with "add", like this:
Add `loader.env.LD_PRELOAD = ...`
Done.
openvino/README.md, line 141 at r2 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Please put this sentence in braces (
(and))
Done.
openvino/README.md, line 143 at r2 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Let's replace with
Modify the manifest file:
Done.
openvino/README.md, line 144 at r2 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Could you add a sub-list for three
fs.mount.usr_locallines? Just like you did forsgx.trusted_files. This will be visually more clear.So it will be something like this:
- Add the `/usr/local` FS mount point: - `fs.mount.usr_local.type = "chroot"` - `fs.mount.usr_local.path = "/usr/local"` - `fs.mount.usr_local.uri = "file:/usr/local"`
Done.
openvino/README.md, line 147 at r2 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Let's prepend with "add", like this:
Add `loader.env.LD_PRELOAD = ...`
Done.
dimakuv
left a comment
dimakuv
left a comment
There was a problem hiding this comment.
Reviewed 3 of 3 files at r3, all commit messages.
Reviewable status: all files reviewed, 14 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @dimakuv and @sahason)
openvino/.gitignore, line 9 at r3 (raw file):
*.manifest.sgx *.sig *.token
Please remove the last four items. These patterns are already covered by a top-level .gitignore file: https://github.com/gramineproject/examples/blob/master/.gitignore#L2-L5
(Recall that .gitignore files accumulate through directories. So the top-level file contents are appended to the contents of your file.)
openvino/README.md, line 93 at r2 (raw file):
Previously, sahason wrote…
I will remove precision. It mostly depends on the model size of different models. For bert-large it requires 64 GB and for brain-tumor-segmentation-0001 and 0002 it requires 128 GB memory.
No for latency it does not change based on the model. Options
-nireq,-nstreamsand-nthreadsare set asnumber of physical cores * 2for throughput capturing whereas these options are set as 1 by default for latency capturing hence for latency 32 GB enclave size is enough for all the models.
Thanks for the explanation!
openvino/README.md, line 96 at r2 (raw file):
Previously, sahason wrote…
Yes.
Ok, what about this re-phrasing then:
... brain-tumor-segmentation-0001 FP32/FP16 models, add more NUMA nodes using
`numactl --membind` if memory allocation fails (for throughput runs).
(Please also re-wrap my suggested text to 80 chars)
openvino/README.md, line 62 at r3 (raw file):
#### Native To run the benchmark in a native baremetal (outside Gramine), replace `gramine-sgx benchmark_app`
in a native baremetal -> natively
openvino/README.md, line 78 at r3 (raw file):
#### Native To run the benchmark in a native baremetal (outside Gramine), replace `gramine-sgx benchmark_app`
in a native baremetal -> natively
openvino/README.md, line 85 at r3 (raw file):
- The models require ~3GB of disk space. - After setting up OpenVINO environment variables if you want to build Gramine after cleaning you need to unset `LD_LIBRARY_PATH`. Please make sure to set up OpenVINO environment
to build Gramine after cleaning -> to re-build Gramine
openvino/README.md, line 88 at r3 (raw file):
variables after building Gramine again. - To get `number of physical cores`, do `lscpu | grep 'Core(s) per socket'`. - Option `i <image files>` is optional. A user may use this option as required.
i -> -i (you forgot the minus sign)
openvino/README.md, line 88 at r3 (raw file):
variables after building Gramine again. - To get `number of physical cores`, do `lscpu | grep 'Core(s) per socket'`. - Option `i <image files>` is optional. A user may use this option as required.
A user may use this option as required. -- this phrase is obvious. Either delete this sentence, or make it explicit (like, the user may use this option to benchmark specific images rather than randomly generated ones.)
openvino/README.md, line 91 at r3 (raw file):
- Please tune the batch size to get the best performance on your system. - Model files for bert-large can be found in `model/intel` directory and for rest of the models these are stored in `model/public` directory.
This sentence sounds weird. Make it more uniform:
- Models for bert-large can be found in `model/intel` directory; the rest of
the models can be found in `model/public` directory.
openvino/README.md, line 93 at r3 (raw file):
the models these are stored in `model/public` directory. - For bert-large and brain-tumor-segmentation models the enclave size must be set to 64/128 GB for capturing throughput.
I dislike this for capturing throughput. Please replace with for throughput runs
openvino/README.md, line 115 at r3 (raw file):
- Preheat manifest option pre-faults the enclave memory and moves the performance penalty to gramine-sgx startup (before the workload starts executing). To use the preheat option, make sure
gramine-sgx -> Gramine-SGX
openvino/README.md, line 124 at r3 (raw file):
TCMalloc and mimalloc are memory allocator libraries from Google and Microsoft that can help improve performance significantly based on the workloads. At any point, only one of these
Just remove At any point,; it doesn't add any value to the sentence.
openvino/README.md, line 131 at r3 (raw file):
(Please update the binary location and name if different from default.) - Install tcmalloc: `sudo apt-get install google-perftools` - Modify the manifest file:
Oops, forgot the template word. Please add like this: Modify the manifest template file:
openvino/README.md, line 142 at r3 (raw file):
(Please update the binary location and name if different from default.) - Install mimalloc using the steps from https://github.com/microsoft/mimalloc - Modify the manifest file:
Oops, forgot the template word. Please add like this: Modify the manifest template file:
sahason
left a comment
sahason
left a comment
There was a problem hiding this comment.
Reviewable status: 4 of 6 files reviewed, 14 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @dimakuv and @sahason)
openvino/.gitignore, line 9 at r3 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Please remove the last four items. These patterns are already covered by a top-level
.gitignorefile: https://github.com/gramineproject/examples/blob/master/.gitignore#L2-L5(Recall that
.gitignorefiles accumulate through directories. So the top-level file contents are appended to the contents of your file.)
Done.
dimakuv
left a comment
dimakuv
left a comment
There was a problem hiding this comment.
Reviewed 2 of 2 files at r4, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @dimakuv and @sahason)
openvino/README.md, line 75 at r4 (raw file):
- The models require ~3GB of disk space. - After setting up OpenVINO environment variables if you want to re-build gramine you need to unset `LD_LIBRARY_PATH`. Please make sure to set up OpenVINO environment
gramine -> Gramine.
Also, please re-wrap to 80 chars per line.
openvino/README.md, line 78 at r4 (raw file):
variables after building Gramine again. - To get `number of physical cores`, do `lscpu | grep 'Core(s) per socket'`. - Option `-i <image files>` is optional. The user may use this option to benchmark specific images rather than randomly generated ones.
please re-wrap to 80 chars per line.
sahason
left a comment
sahason
left a comment
There was a problem hiding this comment.
Reviewable status: 5 of 6 files reviewed, 3 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @dimakuv)
openvino/README.md, line 96 at r2 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
Ok, what about this re-phrasing then:
... brain-tumor-segmentation-0001 FP32/FP16 models, add more NUMA nodes using `numactl --membind` if memory allocation fails (for throughput runs).(Please also re-wrap my suggested text to 80 chars)
Done
openvino/README.md, line 75 at r4 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
gramine->Gramine.Also, please re-wrap to 80 chars per line.
Done.
openvino/README.md, line 78 at r4 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
please re-wrap to 80 chars per line.
Done
dimakuv
left a comment
dimakuv
left a comment
There was a problem hiding this comment.
Reviewed 1 of 1 files at r5, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @dimakuv)
mkow
left a comment
mkow
left a comment
There was a problem hiding this comment.
Reviewed 2 of 5 files at r1, 1 of 3 files at r2, 1 of 3 files at r3, 1 of 2 files at r4, 1 of 1 files at r5, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel), "fixup! " found in commit messages' one-liners (waiting on @dimakuv and @sahason)
openvino/Makefile, line 63 at r5 (raw file):
&& deactivate $(VENV_DIR):
Directory dependencies have some weird semantics in Make, better to use an artificial $(VENV_DIR)/.INSTALLATION_OK or something like this (but you'll have to declare it as PRECIOUS or INTERMEDIATE, I don't remember which one was correct, you need to read Make docs).
Also, the current version will end up in a corrupted state if any of the commands after mkdir fail - next builds will incorrectly succeed.
openvino/README.md, line 20 at r5 (raw file):
## Supported models for Gramine-SGX The following models have been enabled and tested with Gramine-SGX:
enabled and tested -> tested (not sure what "enabled" means in this context)
openvino/README.md, line 42 at r5 (raw file):
Options `-nireq`, `-nstreams` and `-nthreads` should be set to the `number of physical cores * 2` (take into account hyperthreading) for achieving maximum
take into account hyperthreading -> to take hyperthreading into account
openvino/README.md, line 42 at r5 (raw file):
Options `-nireq`, `-nstreams` and `-nthreads` should be set to the `number of physical cores * 2` (take into account hyperthreading) for achieving maximum
Why not just asking the user to use the number of logical/hyper threads here? HT may be disabled on the server b/c of side channel vulnerabilities and they should know what hyper-threading is.
openvino/README.md, line 46 at r5 (raw file):
```bash $ export OPTIMAL_VALUE=<number of physical cores * 2>
OPTIMAL_VALUE -> THREADS_CNT
openvino/README.md, line 116 at r5 (raw file):
TCMalloc and mimalloc are memory allocator libraries from Google and Microsoft that can help improve performance significantly based on the workloads. Only one of these
can be used -> can be used at the same time
sahason
left a comment
sahason
left a comment
There was a problem hiding this comment.
Reviewable status: 4 of 6 files reviewed, 7 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @dimakuv and @mkow)
openvino/Makefile, line 63 at r5 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
Directory dependencies have some weird semantics in Make, better to use an artificial
$(VENV_DIR)/.INSTALLATION_OKor something like this (but you'll have to declare it asPRECIOUSorINTERMEDIATE, I don't remember which one was correct, you need to read Make docs).
Also, the current version will end up in a corrupted state if any of the commands aftermkdirfail - next builds will incorrectly succeed.
Done.
openvino/README.md, line 20 at r5 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
enabled and tested->tested(not sure what "enabled" means in this context)
Done.
openvino/README.md, line 42 at r5 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
take into account hyperthreading->to take hyperthreading into account
replaced with number of logical cores.
openvino/README.md, line 42 at r5 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
Why not just asking the user to use the number of logical/hyper threads here? HT may be disabled on the server b/c of side channel vulnerabilities and they should know what hyper-threading is.
Done.
openvino/README.md, line 46 at r5 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
OPTIMAL_VALUE->THREADS_CNT
Done.
openvino/README.md, line 116 at r5 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
can be used->can be used at the same time
Done.
mkow
left a comment
mkow
left a comment
There was a problem hiding this comment.
Reviewed 2 of 2 files at r6, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @dimakuv and @sahason)
openvino/README.md, line 42 at r5 (raw file):
Previously, sahason wrote…
replaced with number of logical cores.
I think the correct term is "logical cores", not "logical threads"?
openvino/README.md, line 46 at r5 (raw file):
Previously, sahason wrote…
Done.
Please use "logical cores" as above, this calculation isn't always correct (as I said above, you can have HT disabled).
openvino/README.md, line 78 at r6 (raw file):
OpenVINO environment variables after building Gramine again. - To get `Core(s) per socket`, do `lscpu | grep 'Core(s) per socket'`. - To get `Thread(s) per core`, do `lscpu | grep 'Thread(s) per core'`.
This will also need an update after you fix my remarks above.
sahason
left a comment
sahason
left a comment
There was a problem hiding this comment.
Reviewable status: 5 of 6 files reviewed, 4 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @dimakuv and @mkow)
openvino/README.md, line 42 at r5 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
I think the correct term is "logical cores", not "logical threads"?
Here the optimal value for best performance is found as the number of logical cores in one socket not the total number of logical cores in a multi-socket system. So I have changed this as logical cores per socket.
openvino/README.md, line 46 at r5 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
Please use "logical cores" as above, this calculation isn't always correct (as I said above, you can have HT disabled).
Here the optimal value for best performance is found as the number of logical cores in one socket. Even if HT disabled Thread(s) per core will be 1. So this calculation seems fine.
openvino/README.md, line 78 at r6 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
This will also need an update after you fix my remarks above.
Here the optimal value for best performance is found as the number of logical cores in one socket. Even if HT is disabled Thread(s) per core will be 1. So this calculation seems fine.
mkow
left a comment
mkow
left a comment
There was a problem hiding this comment.
Reviewed 1 of 1 files at r7, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @dimakuv, @mkow, and @sahason)
openvino/README.md, line 42 at r5 (raw file):
Previously, sahason wrote…
Here the optimal value for best performance is found as the number of logical cores in one socket not the total number of logical cores in a multi-socket system. So I have changed this as logical cores per socket.
Hmm, does this mean that not using the other CPU at all is faster than using it?
sahason
left a comment
sahason
left a comment
There was a problem hiding this comment.
Reviewable status: all files reviewed, 4 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @dimakuv and @mkow)
openvino/README.md, line 42 at r5 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
Hmm, does this mean that not using the other CPU at all is faster than using it?
Please note the usage of numactl --cpubind=0 --membind=0 in this command. So this is executing the command only on node 0. On a 36 cores system, the number of logical cores available to run this command will be 36 if hyperthreading is disabled and 72 if hyperthreading is enabled. The best performance can be achieved if we run 36 threads if hyperthreading is disabled and 72 threads if hyperthreading is enabled. Hence the THREAD_CNT is calculated as Core(s) per socket * Thread(s) per core. As per the command it is using all the allocated CPUs.
mkow
left a comment
mkow
left a comment
There was a problem hiding this comment.
Reviewable status: all files reviewed, 4 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @dimakuv, @mkow, and @sahason)
openvino/README.md, line 42 at r5 (raw file):
Previously, sahason wrote…
Please note the usage of
numactl --cpubind=0 --membind=0in this command. So this is executing the command only on node 0. On a 36 cores system, the number of logical cores available to run this command will be 36 if hyperthreading is disabled and 72 if hyperthreading is enabled. The best performance can be achieved if we run 36 threads if hyperthreading is disabled and 72 threads if hyperthreading is enabled. Hence the THREAD_CNT is calculated asCore(s) per socket * Thread(s) per core. As per the command it is using all the allocated CPUs.
Then you should write "logical cores on the socket 0", that's the number you want here. Also, could you make it clear that this example is utilizing only socket0? This is quite important and I missed this when reading the current version.
sahason
left a comment
sahason
left a comment
There was a problem hiding this comment.
Reviewable status: 5 of 6 files reviewed, 4 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @dimakuv and @mkow)
openvino/README.md, line 42 at r5 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
Then you should write "logical cores on the socket 0", that's the number you want here. Also, could you make it clear that this example is utilizing only socket0? This is quite important and I missed this when reading the current version.
Done.
mkow
left a comment
mkow
left a comment
There was a problem hiding this comment.
Reviewed 1 of 1 files at r8, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @dimakuv and @sahason)
openvino/README.md, line 46 at r5 (raw file):
Previously, sahason wrote…
Here the optimal value for best performance is found as the number of logical cores in one socket. Even if HT disabled Thread(s) per core will be 1. So this calculation seems fine.
What about here?
openvino/README.md, line 78 at r6 (raw file):
Previously, sahason wrote…
Here the optimal value for best performance is found as the number of logical cores in one socket. Even if HT is disabled Thread(s) per core will be 1. So this calculation seems fine.
And here?
sahason
left a comment
sahason
left a comment
There was a problem hiding this comment.
Reviewable status: 5 of 6 files reviewed, 3 unresolved discussions, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ), "fixup! " found in commit messages' one-liners (waiting on @dimakuv and @mkow)
openvino/README.md, line 46 at r5 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
What about here?
Done.
openvino/README.md, line 78 at r6 (raw file):
Previously, mkow (Michał Kowalczyk) wrote…
And here?
Done.
mkow
left a comment
mkow
left a comment
There was a problem hiding this comment.
Reviewed 1 of 1 files at r9, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @dimakuv)
This commit replaces the old OpenVINO `object_detection_sample_ssd` example. Now the OpenVINO example may be used for benchmarking purposes. Signed-off-by: Sonali Saha <sonali.saha@intel.com>
dimakuv
left a comment
dimakuv
left a comment
There was a problem hiding this comment.
Reviewed 1 of 2 files at r6, all commit messages.
Reviewable status: all files reviewed, 7 unresolved discussions, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: ITL), "fixup! " found in commit messages' one-liners (waiting on @dimakuv and @sahason)
openvino/benchmark_app.manifest.template, line 1 at r9 (raw file):
loader.preload = "file:{{ gramine.libos }}"
TODO(myself): add pal.entrypoint and mark this preload option as "for compatibility"
openvino/benchmark_app.manifest.template, line 35 at r9 (raw file):
sgx.preheat_enclave = true loader.insecure__use_cmdline_argv = true
TODO(myself): separate this visible from other options (move up)
openvino/Makefile, line 11 at r9 (raw file):
INFERENCE_ENGINE_CPP_SAMPLES_BUILD ?= $(THIS_DIR)inference_engine_cpp_samples_build MODEL_DIR ?= $(THIS_DIR)model
TODO(myself): add / for readability
openvino/README.md, line 33 at r9 (raw file):
1. Set up OpenVINO environment variables by running: - root user: `source /opt/intel/openvino_2021/bin/setupvars.sh` - root user and set permanently: Append `source /opt/intel/openvino_2021/bin/setupvars.sh` to `~/.bashrc`
TODO(myself): Append -> append
openvino/README.md, line 44 at r9 (raw file):
Options `-nireq`, `-nstreams` and `-nthreads` should be set to the `number of logical cores on the socket 0` for achieving maximum performance.
TODO(myself): remove backticks, this is just normal text. Maybe use italics.
openvino/README.md, line 71 at r9 (raw file):
To run the benchmark natively (outside Gramine), replace `gramine-sgx benchmark_app` with `./benchmark_app` in the above command.
TODO(myself): Add a similar sentence to run with gramine-direct. Maybe under a separate subsection.
dimakuv
force-pushed
the
sahason/ov_benchmark
branch
3 times, most recently
from
d2a8ddc to
7ddf010
Compare
dimakuv
left a comment
dimakuv
left a comment
There was a problem hiding this comment.
Reviewed 2 of 3 files at r10, 1 of 1 files at r11, all commit messages.
Reviewable status: all files reviewed, all discussions resolved, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel)
Previously, sahason wrote…
OK.
Done
openvino/benchmark_app.manifest.template, line 1 at r9 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
TODO(myself): add
pal.entrypointand mark this preload option as "for compatibility"
Done
openvino/benchmark_app.manifest.template, line 35 at r9 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
TODO(myself): separate this visible from other options (move up)
Done
openvino/Makefile, line 11 at r9 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
TODO(myself): add
/for readability
Done
openvino/README.md, line 33 at r9 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
TODO(myself):
Append->append
Done
openvino/README.md, line 44 at r9 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
TODO(myself): remove backticks, this is just normal text. Maybe use italics.
Done
openvino/README.md, line 71 at r9 (raw file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
TODO(myself): Add a similar sentence to run with
gramine-direct. Maybe under a separate subsection.
Done
dimakuv
left a comment
dimakuv
left a comment
There was a problem hiding this comment.
Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @sahason)
a discussion (no related file):
@sahason I rebased this PR to the latest master. It is ready to merge. Could you please verify that this version is still correct? Could you please verify that OpenVINO builds and runs fine with Gramine v1.1 release?
There was a problem hiding this comment.
Reviewable status: all files reviewed, 1 unresolved discussion, not enough approvals from maintainers (2 more required), not enough approvals from different teams (2 more required, approved so far: ) (waiting on @dmitrii and @mkow)
a discussion (no related file):
Previously, dimakuv (Dmitrii Kuvaiskii) wrote…
@sahason I rebased this PR to the latest master. It is ready to merge. Could you please verify that this version is still correct? Could you please verify that OpenVINO builds and runs fine with Gramine v1.1 release?
Thanks Dmitrii. I have verified that OpenVINO builds and runs fine with the Gramine v1.1 release and the version is correct. Please go ahead and merge. Thank you Dmitrii, @mkow for your input.
dimakuv
left a comment
dimakuv
left a comment
There was a problem hiding this comment.
Reviewable status: all files reviewed, all discussions resolved, not enough approvals from maintainers (1 more required), not enough approvals from different teams (1 more required, approved so far: Intel)
a discussion (no related file):
Previously, sahason wrote…
Thanks Dmitrii. I have verified that OpenVINO builds and runs fine with the Gramine v1.1 release and the version is correct. Please go ahead and merge. Thank you @dmitrii, @mkow for your input.
Thanks! Approving.
mkow
left a comment
mkow
left a comment
There was a problem hiding this comment.
Reviewed 2 of 3 files at r10, 1 of 1 files at r11, all commit messages.
Reviewable status:complete! all files reviewed, all discussions resolved
3 participants
Signed-off-by: Sonali Saha sonali.saha@intel.com
Description of the changes
Examples to run "Benchmark C++ Tool" (benchmark_app) from the OpenVINO distribution under Graphene-SGX to estimate deep learning inference performance.
How to test this PR?
Please follow the steps present in
openvino/README.mdThis change is