Kimmy/dpe benchmark #136
Kimmy/dpe benchmark #136
Conversation
Pull Request Test Coverage Report for Build 581978162
💛 - Coveralls |
src/utils.h
Outdated
| * | ||
| * @param total_target Total number of target. | ||
| * | ||
| * @param even_dist The device distribution is homogeneous or not. |
There was a problem hiding this comment.
The docs say even_dist but the argument is named homo_dist.
src/utils.cc
Outdated
| namespace testing { | ||
|
|
||
| /** Use Megabytes */ | ||
| std::vector<i64> device_size {128, 1024, 4096, 16384}; |
There was a problem hiding this comment.
Since this is only used in one function (InitDeviceState) can you define it in that function instead of using a global variable?
src/utils.cc
Outdated
| std::vector<i64> device_size {128, 1024, 4096, 16384}; | ||
|
|
||
| /** Use Megabytes/Sec */ | ||
| std::vector<double> device_bandwidth {8192, 3072, 550, 120}; |
There was a problem hiding this comment.
Same comment as device_size.
src/utils.cc
Outdated
| std::vector<double> tgt_homo_dist {0.25, 0.25, 0.25, 0.25}; | ||
| std::vector<double> tgt_heto_dist {0.1, 0.2, 0.3, 0.4}; |
There was a problem hiding this comment.
Same comment as device_size.
src/utils.cc
Outdated
| std::vector<size_t> GenFixedNumberOfBlobs(int num, | ||
| size_t each_blob_size) { | ||
| std::vector<size_t> result; | ||
|
|
||
| for (auto i {0}; i < num; ++i) { | ||
| result.push_back(each_blob_size); | ||
| } | ||
| return result; | ||
| } |
There was a problem hiding this comment.
This functionality can be achieved more efficiently (in terms of amount of code and number of instructions) with the std::vector constructor that takes a size and a default value: std::vector<size_t> vec(num, each_blob_size);. I would just replace all calls to GetFixedNumberOfBlobs(num, each_blob_size); with std::vector<T>(num, each_blob_size).
There was a problem hiding this comment.
Right, this function is silly...
benchmarks/dpe_bench.cc
Outdated
| using std::chrono::time_point; | ||
| const auto now = std::chrono::high_resolution_clock::now; | ||
|
|
||
| enum class PlacementPolicy { |
There was a problem hiding this comment.
Can't you use hermes::api::PlacementPolicy instead of redefining the policies here?
benchmarks/dpe_bench.cc
Outdated
| #define TOTAL_TARGETS 10 | ||
| #define BLOB_RANGE testing::BlobSizeRange::kHuge | ||
|
|
||
| #define TOTAL_NUM_BLOBS 10 | ||
| #define TOTAL_BLOB_SIZE GIGABYTES(10) |
There was a problem hiding this comment.
The Google style suggests preferring const variables over macros.
benchmarks/dpe_bench.cc
Outdated
|
|
||
| assert(tgt_state.num_targets == TOTAL_TARGETS); | ||
| std::cout << "Initialize Device State.\n"; | ||
| /* testing::PrintNodeState(tgt_state); */ |
There was a problem hiding this comment.
Is this debug code that can be removed?
benchmarks/dpe_bench.cc
Outdated
| } | ||
|
|
||
| std::cout << "Blob placement is done.\n"; | ||
| /* testing::PrintNodeState(tgt_state); */ |
There was a problem hiding this comment.
Is this debug code that can be removed?
No description provided.