Skip to content

Regain performance by caching initializer names in ORTModule#7685

Merged
baijumeswani merged 3 commits intomasterfrom
bmeswani/perf-regression
May 14, 2021
Merged

Regain performance by caching initializer names in ORTModule#7685
baijumeswani merged 3 commits intomasterfrom
bmeswani/perf-regression

Conversation

@baijumeswani
Copy link
Copy Markdown
Contributor

@baijumeswani baijumeswani commented May 13, 2021

Every call to self._graph_info.initializer_names or self._graph_info.initializer_names_to_train resulted in a O(n) lookup time. Although these two were Python sets, their lookup time was linear. This was happening most likely because on every reference, pybind made a fresh copy of the C++ unordered_set to a Python set.

While we research ways to optimize this, this pull request fixes the perf regression that #7631 introduced by caching the set of initalizer names on the frontend.

This pull request also adds more unit tests for testing the support for unused model parameters.

Copy link
Copy Markdown
Contributor

@thiagocrepaldi thiagocrepaldi left a comment

Choose a reason for hiding this comment

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

LGTM
I am assuming you ran Ravi's script which benchmarks all scenarios

@baijumeswani
Copy link
Copy Markdown
Contributor Author

LGTM
I am assuming you ran Ravi's script which benchmarks all scenarios

I ran Ravi's script for one of the models and it looked good. I am assuming this will generalize to other models as well.

@mrry
Copy link
Copy Markdown
Contributor

mrry commented May 13, 2021

Thanks for figuring out the problem Baiju! I would not have expected that property access to be causing a set construction every time :).

We should definitely merge this as-is, but one possibility for fixing the TODO is to investigate the trick @codemzs used to avoid copying STL structures (vectors in this case) across the Python/C++ boundary:

PYBIND11_MAKE_OPAQUE(std::vector<OrtValue>);

@ytaous
Copy link
Copy Markdown
Contributor

ytaous commented May 13, 2021

do u know what's perf gain by % with Ravi's?


In reply to: 840811250

@baijumeswani
Copy link
Copy Markdown
Contributor Author

@ytaous no additional gain. Just back to where we were before pull request #7631 caused the regression.

@baijumeswani baijumeswani merged commit 37f69fc into master May 14, 2021
@baijumeswani baijumeswani deleted the bmeswani/perf-regression branch May 14, 2021 03:54
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.

4 participants