Technical debt changes #1957
Merged
Technical debt changes #1957
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
…eaders Using anonymous namespace in header files is against best C++ practices, since entities in anonymous namespace have internal linkage, and every translation unit that includes the header file would have its own copy, increasing compilation time and bloating the binary size.
Using anonymous namespace in headers is against best C++ practices due to internal linkage of entities in that namespace.
Avoid using comparator type to form kernel name types for iota and map_back kernels (as they do not depedent on comparator). This reduces the number of kernels generated during instantiation of template implementation functions.
Since vectors `ptrs` and `dels` are no longer needed after host_task submission, we might as well avoid the copying and use std::move in lambda capture initialization. Also renamed `Args` template pack to `UniquePtrTs`, and `args` template argument to `unique_ptrs`. Added comments next to each include to note the entity which requires it.
…late params Removed unncessary template parameters from kernel names submitted by these functions. As a consequence, the size of `_tensor_accumulation_impl` shared object reduced from 49'360'152 bytes to 36'422'888, that is, by almost 13MB.
|
@AlexanderKalistratov I pushed the changes you suggested to make in |
|
Deleted rendered PR docs from intelpython.github.com/dpctl, latest should be updated shortly. 🤞 |
|
Array API standard conformance tests for dpctl=0.19.0dev0=py310h93fe807_415 ran successfully. |
|
The changes in this PR have also reduced the build time of dpctl, by about 10 minutes for me this branch master |
AlexanderKalistratov
approved these changes
Jan 9, 2025
Direct calls to host_task to asynchronously deallocate USM temporary are replaced with call to async_smart_free which submits the host_task for us and transfers allocation ownership from smart pointer to the host task.
…_generic_impl to take packed shape/strides as const pointer
The unique_ptr owns the allocation ensuring no leaks during exception handling. This also allows async_smart_free to be used to schedule asynchronous deallocation of USM temporaries.
|
Array API standard conformance tests for dpctl=0.19.0dev0=py310h93fe807_420 ran successfully. |
Doing so reduces the binary size. Previously, '_tensor_sorting_impl' module has size 22'448'920 bytes, and '_tensor_sorting_radix_impl' has size 31'927'256 bytes. Total size was 54'376'176 bytes. After this change, the total size of the new '_tensor_sorting_impl' is 49'790'872, which is about 4Mb of savings.
|
Array API standard conformance tests for dpctl=0.19.0dev0=py310h93fe807_421 ran successfully. |
ndgrigorian
approved these changes
Jan 10, 2025
6 tasks
antonwolfy
added a commit
to IntelPython/dpnp
that referenced
this pull request
Jan 11, 2025
The PR proposes to implement fixes/improvements into the public CI: - run `pre-commit` and `building docs` workflows on Ubuntu-22.04 (`ubuntu-latest` now refers to Ubuntu-24.04 where the workflows fail as for now) - use `fail-fast: false` rather than `continue-on-error: false` in workflow with night tests run (some jobs in strategy matrix may fail due to mamba/conda issues) _Note, the workflow will be reported as failed if any job fails._ - trigger rerun tests step on failure of `run_tests` step only in night tests workflow - trigger resetup minoconda step on failure in night tests and conda package workflows - build name of running job based on python and OS from strategy matrix - disable night tests workflow to be run in forked repositories Also the PR includes adaptation to the recent interface changes in dpctl implemented in [gh-1957](IntelPython/dpctl#1957).
github-actions bot
added a commit
to IntelPython/dpnp
that referenced
this pull request
Jan 11, 2025
The PR proposes to implement fixes/improvements into the public CI: - run `pre-commit` and `building docs` workflows on Ubuntu-22.04 (`ubuntu-latest` now refers to Ubuntu-24.04 where the workflows fail as for now) - use `fail-fast: false` rather than `continue-on-error: false` in workflow with night tests run (some jobs in strategy matrix may fail due to mamba/conda issues) _Note, the workflow will be reported as failed if any job fails._ - trigger rerun tests step on failure of `run_tests` step only in night tests workflow - trigger resetup minoconda step on failure in night tests and conda package workflows - build name of running job based on python and OS from strategy matrix - disable night tests workflow to be run in forked repositories Also the PR includes adaptation to the recent interface changes in dpctl implemented in [gh-1957](IntelPython/dpctl#1957). 303a203
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR checks in some technical debt changes.
Rationale: Entities defined in anonymous namespace have internal linkage, that is, they are generated in every translation units that includes the header file bloating the size of compiled binary, as well increasing compilation time
Rationale: Inline use requires kernel names to be distinct for function different instantiations, producing multiple copies of identical kernels, bloating the binary.
This change alone shed 13MB off the size of
_tensor_accumulation_implnative extension (from 49MB to 36MB).