fix: avoid additional array allocation in host to device transfer #2966
Conversation
|
/intelci: run |
Alexandr-Solovev
changed the title
Alexandr-Solovev
requested review from
Alexsandruss and
samir-nasibli
as code owners
Alexandr-Solovev
changed the title
|
Please dont merge, until the additional check on local machines |
not sure, looks like in all places where the function is called there is additional wait_and_throw() |
|
/intelci: run |
|
/intelci: run |
|
/intelci: run |
Alexandr-Solovev
changed the title
| @@ -130,33 +130,20 @@ static void pull_row_major_impl(const Policy& policy, | |||
|
|
|||
| auto src_data = origin_data.get_data() + origin_offset * origin_dtype_size; | |||
| auto dst_data = block_data.get_mutable_data(); | |||
|
|
|||
| if (block_info.get_column_count() > 1) { | |||
There was a problem hiding this comment.
Just because I am not familiar, it looks like just removing the if and relying on the for loop (a simplification here).
| dal::detail::check_mul_overflow(src_element_size_in_bytes, element_count); | ||
| const std::int64_t src_stride_in_bytes = | ||
| dal::detail::check_mul_overflow(src_element_size_in_bytes, src_stride); | ||
| if (src_type == dst_type && src_size_in_bytes == dst_size_in_bytes && |
There was a problem hiding this comment.
If allocation exists, skip the convert_convert vector and uses memcpy_host2usm
There was a problem hiding this comment.
The issue, that when we copy from host to device in different data types(float on host and allocated on device in double), we still just copy bytes, and in the scenario of usage different data types it makes sense to use strides
| @@ -227,33 +227,50 @@ sycl::event convert_vector_device2host(sycl::queue& q, | |||
| // To perform conversion, we gather data from device to host in temporary | |||
| // contigious array and then run host conversion function | |||
|
|
|||
| const std::int64_t element_size_in_bytes = dal::detail::get_data_type_size(src_type); | |||
| const std::int64_t dst_element_size_in_bytes = dal::detail::get_data_type_size(dst_type); | |||
There was a problem hiding this comment.
collect more information about the destination to see if a shortcut can be used.
There was a problem hiding this comment.
Indeed good suggestion! Thank you @Alexandr-Solovev
Could you please share perf improvements?
|
/intelci: run |
|
Are you still seeing seg faults when experimenting locally or has this issue been resolved? |
|
/intelci: run |
Description
PR introduces fix for memcpy function in oneDAL. It saves memory and improve the performance of transferring data from host to device and back and gives an opportunity to use memcpy directly.
PR should start as a draft, then move to ready for review state after CI is passed and all applicable checkboxes are closed.
This approach ensures that reviewers don't spend extra time asking for regular requirements.
You can remove a checkbox as not applicable only if it doesn't relate to this PR in any way.
For example, PR with docs update doesn't require checkboxes for performance while PR with any change in actual code should have checkboxes and justify how this code change is expected to affect performance (or justification should be self-evident).
Checklist to comply with before moving PR from draft:
PR completeness and readability
Testing
Performance