Skip to content

Use protobuf-lite to reduce onnxruntime.dll size.#639

Merged
pranavsharma merged 20 commits into
masterfrom
protobuf_lite
Mar 21, 2019
Merged

Use protobuf-lite to reduce onnxruntime.dll size.#639
pranavsharma merged 20 commits into
masterfrom
protobuf_lite

Conversation

@pranavsharma
Copy link
Copy Markdown
Contributor

@pranavsharma pranavsharma commented Mar 15, 2019

More details about the LITE_RUNTIME can be found here https://developers.google.com/protocol-buffers/docs/proto. The reduction is significant. For commit id: 4873b452151bafe49da332aaeab639ef0318fc1ca28d728, the size reduced by 700K; from 4873728 to 4172800 bytes.

  • Introduce a new build option "use_full_protobuf" so that onnx-tensorrt converter doesn't need to accommodate the protobuf-lite related changes. This option is OFF by default for non-tensorrt builds.
  • Introduced 2 new files protobuf_parsing_utils.h/cc that is a copy of zero_copy_stream_impl.h/cc. These are required when using protobuf-lite to retain the same efficiency in loading the model files. This is based on the suggestion by the protobuf author here: https://groups.google.com/forum/#!topic/protobuf/J3H0GSlrfIo
  • Note that I had to disable 2 test cases since they relied on parsing protobuf represented in text format; protobuf-lite doesn't support this functionality. We can convert the text protobuf to binary if required. As such in our case this functionality is needed for unittests only.

More details on the binary analysis can be found here: https://microsoft-my.sharepoint.com/:w:/p/prs/ERGfQ3IIjWxFtznJCM_nLbwBVrziUgzcvNN9T3a6ETDIQw?e=OydgWL

Pranav Sharma added 6 commits March 13, 2019 20:09
@pranavsharma pranavsharma requested a review from a team as a code owner March 15, 2019 23:15
@snnn
Copy link
Copy Markdown
Contributor

snnn commented Mar 15, 2019

Any performance change?

Comment thread onnxruntime/core/protobuf/onnx-ml.proto Outdated
Comment thread onnxruntime/test/shared_lib/test_model_loading.cc Outdated
Comment thread onnxruntime/test/util/compare_mlvalue.cc Outdated
@pranavsharma
Copy link
Copy Markdown
Contributor Author

pranavsharma commented Mar 15, 2019

Any performance change?

There should be zero change in performance since the model loading functionality has not changed. Previously we were able to take advantage of single APIs: ParseFromIStream and ParseFromFileDescriptor. Now we use the exact same functionality in 2 steps. See inference_session.cc and model.cc.

Comment thread cmake/CMakeLists.txt Outdated
Comment thread cmake/CMakeLists.txt Outdated
@pranavsharma pranavsharma requested a review from a team March 18, 2019 20:34
Comment thread cmake/onnxruntime_unittests.cmake Outdated
Copy link
Copy Markdown
Contributor

@snnn snnn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please take a look at the build log.
It has a lot of error messages like:
"Parsing config failed."

Apparently, your new config parsing code doesn't work.

@pranavsharma
Copy link
Copy Markdown
Contributor Author

Please take a look at the build log.
It has a lot of error messages like:
"Parsing config failed."

Apparently, your new config parsing code doesn't work.

There was no problem with the parsing. The file just doesn't exist for many tests. The error message printed was misleading. I'll remove that line since it made sense only when we did protobuf parsing.

Comment thread onnxruntime/test/providers/provider_test_utils.cc Outdated
Comment thread onnxruntime/test/providers/provider_test_utils.cc
Comment thread tools/ci_build/build.py Outdated
@pranavsharma pranavsharma merged commit 5d452b3 into master Mar 21, 2019
@raymondxyang raymondxyang deleted the protobuf_lite branch March 22, 2019 18:33
yuslepukhin pushed a commit that referenced this pull request Mar 17, 2026
## Describe your changes

1. Skip vitis tests for ORT 1.16.1. Wait for VitisAI teams to fix this
then to add the tests back.
2. Remove quant pre process as the bug is fixed in 1.16.1.
3. lower the metrics goal to allow at least one model in output.

## Checklist before requesting a review
- [ ] Add unit tests for this change.
- [ ] Make sure all tests can pass.
- [ ] Update documents if necessary.
- [ ] Format your code by running `pre-commit run --all-files`
- [ ] Is this a user-facing change? If yes, give a description of this
change to be included in the release notes.

## (Optional) Issue link
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants