update latency configuration for multi numa nodes on one socket #26798
Conversation
github-actions
bot
added
category: inference
wangleis
requested review from
sunxiaoxia2022 and
dmitry-gorokhov
and removed request for
a team
wangleis
force-pushed
the
latency_on_numa_node
branch
from
f59cef6
to
3df35d9
Compare
wangleis
force-pushed
the
latency_on_numa_node
branch
from
fb6b424
to
1128ae4
Compare
|
|
||
| namespace ov { | ||
|
|
||
| void update_table_for_proc(const int _processor_id, |
There was a problem hiding this comment.
Why we need to exposure this function? Do you have expectation developers will be needed to call it explicitly?
I would propose to keep it as private method of CPU class.
Tests should have its own implementation of this utility.
There was a problem hiding this comment.
Move this function into CPU class. Now this is not private method because specific test case for this function.
@dmitry-gorokhov is it ok?
There was a problem hiding this comment.
As I mentioned test should have own implementation of this utility. I don't think this is good practice to exposure some functionality as dev API only in order to access it in the tests.
| @@ -282,6 +283,11 @@ CPU::CPU() { | |||
| OPENVINO_THROW("CPU affinity check failed. No CPU is eligible to run inference."); | |||
| }; | |||
|
|
|||
| if (_proc_type_table.size() > 1) { | |||
| int cur_processor_id = sched_getcpu(); | |||
| update_table_for_proc(cur_processor_id, _proc_type_table, _cpu_mapping_table); | |||
There was a problem hiding this comment.
- Too generic name? It is mostly like: sort_table_by_cpu_id
- We have to add comment why this sort is needed at all.
There was a problem hiding this comment.
Updated function name and added function description.
|
@dmitry-gorokhov Updated code as comments. Please review again. |
| } | ||
| }; | ||
|
|
||
| friend class LinuxSortProcTableTests; |
There was a problem hiding this comment.
Can test class as friend be removed?
Make sort_table_by_cpu_id as public or in test use test function to sort data.
There was a problem hiding this comment.
@praasz Dmitry ask to keep sort_table_by_cpu_id() as private function.
@dmitry-gorokhov any comments on this?
There was a problem hiding this comment.
Yeah, I would prefer to avoid making functionality public just because we want to test it in unit tests.
GoogleTests has some recommendations on it https://github.com/google/googletest/blob/main/docs/advanced.md#testing-private-code. Maybe FRIEND_TEST will work here.
There was a problem hiding this comment.
Yes, we just followed this GoogleTests recommendation. Since the private function in class CPU is defined in namespace ov, we follow the recommendation to use friend class.
Since the test function is only used inside the unit test file, "FRIEND_TEST" is not necessary.
@praasz @dmitry-gorokhov If you do not have concern, this PR will be merged in following days.
… socket (#26944) ### Details: - *update document for latency configuration for multi numa nodes on one socket* - *PR #26798 is code change* ### Tickets: - *CVS-140601*
Details:
Tickets: