Skip to content

Use the file size while reading onnx models. Ensure models are loaded using APIs in model.h for consistency.#4399

Merged
pranavsharma merged 4 commits intomasterfrom
protoblock
Jul 3, 2020
Merged

Use the file size while reading onnx models. Ensure models are loaded using APIs in model.h for consistency.#4399
pranavsharma merged 4 commits intomasterfrom
protoblock

Conversation

@pranavsharma
Copy link
Contributor

@pranavsharma pranavsharma commented Jul 1, 2020

Description: Use the file size while reading onnx models. Ensure models are loaded using APIs in model.h for consistency.

Motivation and Context
This serves as a middle ground between providing a large fixed sized buffer of 4mb (done here and here) and the default of 8kb inside protobuf. The 4mb change causes an unnecessary increase in memory usage for even the smallest of the models due to this and the default is apparently slowing down model loading on Azure machines due to it's slow reads.

@pranavsharma pranavsharma requested a review from a team as a code owner July 1, 2020 23:02
@pranavsharma
Copy link
Contributor Author

This still leaves the Load function that takes model_istream unoptimized as I'm not sure if there's a reliable way to get the size of the file given an istream. We could potentially make the block size configurable, but that requires a whole gamut of changes that touches all bindings, C APIs, etc and I'm not sure if we really require it.

#if GOOGLE_PROTOBUF_VERSION >= 3002000
FileInputStream input(fd);
size_t file_size = 0;
int block_size = -1;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

snnn
snnn previously approved these changes Jul 2, 2020
@pranavsharma pranavsharma merged commit 4df8a1e into master Jul 3, 2020
@pranavsharma pranavsharma deleted the protoblock branch July 3, 2020 00:30
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.

2 participants