Change memory object USM allocation ownership, and make execution asynchronous #1705

oleksandr-pavlyk · 2024-06-04T00:18:33Z

This is PR changes the size and content of the struct behind dpctl.memory._Memory object.

This is backwards incompatible change, and would require rebuilding of downstream projects with native extensions (cython, or pybind11) that use dpctl.

The _Memory object no longer explicitly frees USM allocations it made, but delegates this job to a smart pointer. This allows host_task jobs that ensure deferment of USM deallocation till after offloaded tasks that operate on it complete execution to do their job by capturing the smart pointer in the callable passed to the host_task instead needing to rely on Python reference counting, and thus need to acquire GIL in that callable.

Furthermore, the PR introduces mechanism of ensuring sequential order amongst tasks implementing offloading Python API, such as dpctl.tensor implementing array API compliant tensor library.

One consequence of this change is that timing of dpctl.tensor functions must synchronize the execution queue before taking the final timestamp reading.

Have you provided a meaningful PR description?
Have you added a test, reproducer or referred to an issue with a reproducer?
Have you tested your changes locally for CPU and GPU devices?
Have you made sure that new changes do not introduce compiler warnings?
Have you checked performance impact of proposed changes?
If this PR is a work in progress, are you opening the PR as a draft?

The _Memory object acquires additional field void * _opaque_ptr. This pointer is the pointer to std::shared_ptr<void> with a deleter class to delete USM allocations. The pointer is opaque to retain C-API of Py_MemoryObject. If _opaque_ptr is NULL, refobj field is assumed to be responsible for USM deallocation. api Memory_GetOpaquePointer is added to access the opaque pointer. A test in test_sycl_usm is modified to reflect factual changes in the behavior of _Memory type.

The manager stores a list of events for tasks submitted so far, as well as list of events for host_tasks submitted so far. Every opperation of the manager prunes lists of completed events. `dpctl.utils.SequentialOrderManager` class instance is Python API to work with this class. Every offloading operation calls `.submitted_events` property to get dependent events, and adds computational event and host_task event to the manager using `.add_to_both_events(ht_ev, comp_ev)` method. The destructor of manager synchronizes on outstanding events.

Remove pervasive use of SyclEvent.wait in favor of using SequentialOrderManager to maintain sequential order semantics via ordering of submitted tasks using events.

Report caugh exception to std::cerr by ignore it otherwise.

… scalars Also synchronize in __sycl_usm_array_interface__ attribute, to ensure that `dpctl.memory.as_usm_memory(ary).copy_to_host()` produces expected results.

Move implementation from dpctl/utils/__init__.py into dedicated files.

Instead of relying on SUAI attribute which has to synchronize to get the offset, use `X._element_offset` attribute directly.

github-actions · 2024-06-04T00:54:24Z

Deleted rendered PR docs from intelpython.github.com/dpctl, latest should be updated shortly. 🤞

github-actions · 2024-06-04T00:55:51Z

Array API standard conformance tests for dpctl=0.18.0dev0=py310h15de555_50 ran successfully.
Passed: 890
Failed: 11
Skipped: 91

coveralls · 2024-06-04T00:59:45Z

coverage: 88.057% (+0.1%) from 87.911%
when pulling f4e3b6f on memory-work
into c6f5f79 on master.

github-actions · 2024-06-04T01:23:31Z

Array API standard conformance tests for dpctl=0.18.0dev0=py310h15de555_51 ran successfully.
Passed: 890
Failed: 11
Skipped: 91

github-actions · 2024-06-04T01:25:32Z

Array API standard conformance tests for dpctl=0.18.0dev0=py310h15de555_51 ran successfully.
Passed: 890
Failed: 11
Skipped: 91

Incidentally, the added test also found a bug

github-actions · 2024-06-04T02:22:00Z

Array API standard conformance tests for dpctl=0.18.0dev0=py310h15de555_53 ran successfully.
Passed: 890
Failed: 11
Skipped: 91

github-actions · 2024-06-04T02:29:51Z

Array API standard conformance tests for dpctl=0.18.0dev0=py310h15de555_55 ran successfully.
Passed: 890
Failed: 11
Skipped: 91

oleksandr-pavlyk · 2024-06-04T16:21:51Z

What's missing from this PR that must be addressed before merging:

dpctl_capi documentation must be expanded with new CAPI added, one for usm_ndarray, and one for usm_memory
Execution model explanation in documentation must now reflect that execution is asynchronous
Examples of dpctl for pybind11 and cython must be updated.

oleksandr-pavlyk · 2024-06-05T19:25:36Z

I suggest we expand the documentation with explaining the asynchronous execution in a separate PR.

The important change to realize is that execution of tensor operations is done asynchronously, so to get a proper timing values, one must use queue.wait() before taking the final time stamp reading.

Submission preserves sequential ordering of operations as executed by Python interpreter.

github-actions · 2024-06-05T20:01:26Z

Array API standard conformance tests for dpctl=0.18.0dev0=py310h15de555_57 ran successfully.
Passed: 890
Failed: 11
Skipped: 91

github-actions · 2024-06-05T20:09:16Z

Array API standard conformance tests for dpctl=0.18.0dev0=py310h15de555_57 ran successfully.
Passed: 890
Failed: 11
Skipped: 91

docs/doc_sources/api_reference/dpctl_capi.rst

github-actions · 2024-06-05T20:52:07Z

Array API standard conformance tests for dpctl=0.18.0dev0=py310h15de555_58 ran successfully.
Passed: 890
Failed: 11
Skipped: 91

github-actions · 2024-06-05T20:59:37Z

Array API standard conformance tests for dpctl=0.18.0dev0=py310h15de555_59 ran successfully.
Passed: 890
Failed: 11
Skipped: 91

ndgrigorian

I've looked over the PR and this LGTM, I think that is a good change overall, and bring tangible performance improvements to the project.

Thank you for the work on this @oleksandr-pavlyk !

github-actions · 2024-06-05T21:44:09Z

Array API standard conformance tests for dpctl=0.18.0dev0=py310h15de555_60 ran successfully.
Passed: 889
Failed: 12
Skipped: 91

oleksandr-pavlyk added 16 commits June 3, 2024 17:24

Deploy using shared pointers from keep_args_alive

dc77858

Adding test for Memory_GetOpaquePointer CAPI function

8a53212

Added tests for UsmNDArray_GetUSMData C-API function

cf1cad0

Fixed typo in the comment

9df2b72

Fixed unclosed quote in error pragma

c85571b

Fixed memory leak introduced in new methods of usm_ndarray

08b6dd0

Transition tensor to use SequentialOrderManager

b78698e

Remove pervasive use of SyclEvent.wait in favor of using SequentialOrderManager to maintain sequential order semantics via ordering of submitted tasks using events.

Wrap call to sycl::free in try/catch

9116e73

Report caugh exception to std::cerr by ignore it otherwise.

Fixed missing dependency events

be401a6

Add queue synchronization points in special methods for conversion to…

106c8de

… scalars Also synchronize in __sycl_usm_array_interface__ attribute, to ensure that `dpctl.memory.as_usm_memory(ary).copy_to_host()` produces expected results.

Use manager per queue in tensor implementation

a5a481e

Move implementation from dpctl/utils/__init__.py into dedicated files.

Adds clear method to SequentialOrderManager

3f0f935

Replaced use of synchronizing __sycl_usm_array_interface__ atribute

bdb2f75

Instead of relying on SUAI attribute which has to synchronize to get the offset, use `X._element_offset` attribute directly.

Fix pre-commit

1930287

oleksandr-pavlyk force-pushed the memory-work branch from f885a0f to 1930287 Compare June 4, 2024 00:48

oleksandr-pavlyk added 4 commits June 3, 2024 20:26

Extend test symmetrically to improve coverage

7d3e228

Add tests for order manager

dd28026

Improve coverage of concat_axis_None

71c5b84

Incidentally, the added test also found a bug

Fix for the bug found by test added in previous commit

a4b510c

oleksandr-pavlyk requested review from ndgrigorian and antonwolfy June 4, 2024 11:13

oleksandr-pavlyk requested review from ZzEeKkAa and samir-nasibli June 4, 2024 11:14

Add docs for two new C-API functions added in this branch for 0.18

61f8c54

oleksandr-pavlyk marked this pull request as ready for review June 5, 2024 19:22

Add synchronization points in example

f6a1f06

oleksandr-pavlyk force-pushed the memory-work branch from f31fb28 to f6a1f06 Compare June 5, 2024 19:31

Added wait call to cppclass queue and cppclass even in sycl.pxd

759c1d0

ndgrigorian reviewed Jun 5, 2024

View reviewed changes

docs/doc_sources/api_reference/dpctl_capi.rst Outdated Show resolved Hide resolved

ndgrigorian reviewed Jun 5, 2024

View reviewed changes

docs/doc_sources/api_reference/dpctl_capi.rst Outdated Show resolved Hide resolved

oleksandr-pavlyk added 2 commits June 5, 2024 15:21

Fixed issues found by @ndgrigorian

7bc1ef8

Add comments for usm_ndarray::get_usm_data method

f4e3b6f

ndgrigorian approved these changes Jun 5, 2024

View reviewed changes

oleksandr-pavlyk merged commit e24e263 into master Jun 5, 2024
60 checks passed

oleksandr-pavlyk deleted the memory-work branch June 5, 2024 23:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change memory object USM allocation ownership, and make execution asynchronous #1705

Change memory object USM allocation ownership, and make execution asynchronous #1705

oleksandr-pavlyk commented Jun 4, 2024

github-actions bot commented Jun 4, 2024 •

edited

Loading

github-actions bot commented Jun 4, 2024

coveralls commented Jun 4, 2024 •

edited

Loading

github-actions bot commented Jun 4, 2024

github-actions bot commented Jun 4, 2024

github-actions bot commented Jun 4, 2024

github-actions bot commented Jun 4, 2024

oleksandr-pavlyk commented Jun 4, 2024 •

edited

Loading

oleksandr-pavlyk commented Jun 5, 2024

github-actions bot commented Jun 5, 2024

github-actions bot commented Jun 5, 2024

github-actions bot commented Jun 5, 2024

github-actions bot commented Jun 5, 2024

ndgrigorian left a comment

github-actions bot commented Jun 5, 2024

Change memory object USM allocation ownership, and make execution asynchronous #1705

Change memory object USM allocation ownership, and make execution asynchronous #1705

Conversation

oleksandr-pavlyk commented Jun 4, 2024

github-actions bot commented Jun 4, 2024 • edited Loading

github-actions bot commented Jun 4, 2024

coveralls commented Jun 4, 2024 • edited Loading

github-actions bot commented Jun 4, 2024

github-actions bot commented Jun 4, 2024

github-actions bot commented Jun 4, 2024

github-actions bot commented Jun 4, 2024

oleksandr-pavlyk commented Jun 4, 2024 • edited Loading

oleksandr-pavlyk commented Jun 5, 2024

github-actions bot commented Jun 5, 2024

github-actions bot commented Jun 5, 2024

github-actions bot commented Jun 5, 2024

github-actions bot commented Jun 5, 2024

ndgrigorian left a comment

Choose a reason for hiding this comment

github-actions bot commented Jun 5, 2024

github-actions bot commented Jun 4, 2024 •

edited

Loading

coveralls commented Jun 4, 2024 •

edited

Loading

oleksandr-pavlyk commented Jun 4, 2024 •

edited

Loading