Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[CPU] CACHE_DIR hash optimization #25624

Merged
merged 2 commits into from
Oct 21, 2024
Merged

[CPU] CACHE_DIR hash optimization #25624

merged 2 commits into from
Oct 21, 2024

Conversation

nshchego
Copy link
Contributor

@nshchego nshchego commented Jul 18, 2024 •
edited
Loading

Details:

  • JIT implementation of the hash function in the ConstantWriter

Tickets:

  • 127331

@github-actions github-actions bot added the category: Core OpenVINO Core (aka ngraph) label Jul 18, 2024
@nshchego nshchego force-pushed the cpu/perf/cache_dir_hash branch 2 times, most recently from 3e16126 to bf875ea Compare July 29, 2024 02:45
@nshchego nshchego marked this pull request as ready for review July 29, 2024 02:56
@nshchego nshchego requested review from a team as code owners July 29, 2024 02:56
@nshchego nshchego requested review from itikhono and removed request for a team July 29, 2024 02:56
@nshchego nshchego requested review from a team as code owners August 1, 2024 06:45
@github-actions github-actions bot added category: inference OpenVINO Runtime library - Inference category: CPU OpenVINO CPU plugin category: build OpenVINO cmake script / infra labels Aug 1, 2024
@github-actions github-actions bot removed category: inference OpenVINO Runtime library - Inference category: CPU OpenVINO CPU plugin category: build OpenVINO cmake script / infra labels Aug 1, 2024
ilya-lavrenov
ilya-lavrenov reviewed Aug 1, 2024
View reviewed changes
src/core/src/pass/serialize.cpp Outdated
@@ -131,7 +115,7 @@ class ConstantWriter {
// the same hash for {2, 2} and {0, 128} arrays.
// But even strong hashing algorithms sometimes give collisions.
// Therefore we always have to compare values when finding a match in the hash multimap.
const HashValue hash = hash_combine(ptr_to_write, *new_size);
const HashValue hash = ov::runtime::combine_hash(ptr_to_write, *new_size);
Copy link
Contributor

@ilya-lavrenov ilya-lavrenov Aug 1, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

  1. does this function provide the same results as original hash_combine ?
  2. If yes, could you please add such tests? It's important to preserve the same results for hashes to avoid regeneration of compiled_blobs on customers side with newer OpenVINO release.
  3. What performance gain do you see from this optimization?

Copy link
Contributor

Choose a reason for hiding this comment

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

If this jit implement for hash computation is good enough, can we also extend to other component's hash computation? As we know, there are many duplicated similar combine_hash functions, such as in compilation_context.cpp, ov::util::hash_combine, ov::snippets::pass::<unnamed>::hash_combine(), can we merge them into a single version?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ilya,

  1. No, that is another approach,
  2. Cache is recompiled on each commit change, due to openvino_version and runtime_info contain commit hash,
  3. Please take a look at file Compile_model_time_cache_HASH_FIX_branch_vs_master.xlsx in the ticket 127331.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

River,
It's good point for further optimizations.

praasz
praasz reviewed Aug 1, 2024
View reviewed changes
src/core/reference/include/openvino/reference/utils/combine_hash.hpp Outdated Show resolved Hide resolved
src/core/reference/src/op/utils/jit_generator.cpp Outdated Show resolved Hide resolved
src/core/reference/src/op/utils/combine_hash.cpp Outdated Show resolved Hide resolved
src/core/reference/src/op/utils/combine_hash.cpp Outdated Show resolved Hide resolved
src/core/reference/src/op/utils/combine_hash.cpp Outdated Show resolved Hide resolved
src/core/reference/include/openvino/reference/utils/registers_pool.hpp Outdated Show resolved Hide resolved
src/core/reference/include/openvino/reference/utils/registers_pool.hpp Outdated Show resolved Hide resolved
src/core/reference/include/openvino/reference/utils/registers_pool.hpp Outdated Show resolved Hide resolved
src/core/reference/src/op/utils/combine_hash.cpp Outdated Show resolved Hide resolved
src/core/reference/src/op/utils/combine_hash.cpp Outdated Show resolved Hide resolved
riverlijunjie
riverlijunjie reviewed Aug 2, 2024
View reviewed changes
src/core/reference/src/op/utils/combine_hash.cpp Outdated Show resolved Hide resolved
src/core/reference/src/op/utils/combine_hash.cpp Outdated Show resolved Hide resolved
src/core/src/pass/serialize.cpp Outdated
@@ -131,7 +115,7 @@ class ConstantWriter {
// the same hash for {2, 2} and {0, 128} arrays.
// But even strong hashing algorithms sometimes give collisions.
// Therefore we always have to compare values when finding a match in the hash multimap.
const HashValue hash = hash_combine(ptr_to_write, *new_size);
const HashValue hash = ov::runtime::combine_hash(ptr_to_write, *new_size);
Copy link
Contributor

Choose a reason for hiding this comment

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

If this jit implement for hash computation is good enough, can we also extend to other component's hash computation? As we know, there are many duplicated similar combine_hash functions, such as in compilation_context.cpp, ov::util::hash_combine, ov::snippets::pass::<unnamed>::hash_combine(), can we merge them into a single version?

@github-actions github-actions bot added the category: build OpenVINO cmake script / infra label Aug 5, 2024
@github-actions GitHub Actions
Copy link
Contributor

This PR will be closed in a week because of 2 weeks of no activity.

@nshchego nshchego force-pushed the cpu/perf/cache_dir_hash branch 6 times, most recently from 343cfc8 to 35a0d2e Compare October 18, 2024 08:36
@nshchego nshchego force-pushed the cpu/perf/cache_dir_hash branch 14 times, most recently from d9134d7 to 5218283 Compare October 21, 2024 10:52
@nshchego nshchego added this pull request to the merge queue Oct 21, 2024
Merged via the queue into openvinotoolkit:master with commit 5f4a445 Oct 21, 2024
160 checks passed
@nshchego nshchego deleted the cpu/perf/cache_dir_hash branch October 21, 2024 18:19
a-sidorova added a commit to a-sidorova/openvino that referenced this pull request Oct 23, 2024
@a-sidorova
Revert "[CPU] CACHE_DIR hash optimization (openvinotoolkit#25624)"
c71bd14 
This reverts commit 5f4a445.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ilya-lavrenov ilya-lavrenov ilya-lavrenov left review comments

@riverlijunjie riverlijunjie riverlijunjie left review comments

@praasz praasz praasz approved these changes

@itikhono itikhono Awaiting requested review from itikhono itikhono is a code owner automatically assigned from openvinotoolkit/openvino-ie-transformations-maintainers

Assignees

@ilya-lavrenov ilya-lavrenov

@riverlijunjie riverlijunjie

@praasz praasz

Labels
category: build OpenVINO cmake script / infra category: Core OpenVINO Core (aka ngraph) no_stale Do not mark as stale
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@nshchego @ilya-lavrenov @riverlijunjie @praasz