Exposure Tracked Buffer (first step towards unifying copy-on-write and spilling) #13307

madsbk · 2023-05-08T14:29:21Z

The first step towards unifying copy-on-write and spillable buffers.

This PR re-implement copy-on-write by introducing a ExposureTrackedBuffer and BufferSlice. The idea is that when copy-on-write (and in a follow-up PR later, when spill) is enabled, we use BufferSlice throughout cudf.
BufferSlice is a view of a ExposureTrackedBuffer that implements copy-on-write semantics by tracking the number of BufferSlice that points to the same ExposureTrackedBuffer.

Checklist

I am familiar with the Contributing Guidelines.
New or existing tests cover these changes.
The documentation is up to date with these changes.

cc. @shwina, @vyasr, @galipremsagar, @wence-

wence-

A first pass, haven't fully managed to go through all the logic yet

docs/cudf/source/developer_guide/library_design.md

wence- · 2023-05-10T11:09:18Z

docs/cudf/source/developer_guide/library_design.md

+
+`TenableBuffer` is a subclass of the regular `Buffer` that tracks its "expose" status of its underlying memory. We say that the buffer has been exposed if the device pointer (integer or void*) has been accessed outside of cudf, in which case we have no control over knowing if the data is being modified by a third-party. Additionally, `TenableBuffer` also maintains [weak references](https://docs.python.org/3/library/weakref.html) to every existing `BufferSlice` that points to its underlying memory.
+
+`BufferSlice` is a subclass of `TenableBuffer` that represents a _slice_ of the memory underlying a tenable buffer.


Is subclassing the right thing? (I haven't yet read the implementation). It seems like every BufferSlice has-a TenableBuffer, but that doesn't necessarily imply an is-a relationship.

If it really is a subclass, do we actually need both classes, or can there just be TenableBuffer objects that either own data, or are views of existing data.

In a follow-up PR, SpillableBuffer will inherent from TenableBuffer and BufferSlice._base will point to SpillableBuffer.

Maybe it is better if BufferSlice inherent from Buffer ?

It kind of seems like what we want is ad-hoc, rather than subtype, polymorphism (BufferSlices are not Liskov-substitutable for TenableBuffers everywhere [in particular when constructing new buffer slices]). In which case, is this the time to introduce a buffer Protocol?

I suppose this is similar to the column vs. column_view idea in libcudf. A Buffer (TenableBuffer, SpillableBuffer) is the concrete owning object, and then the BufferSlice is a non-owning view? Or how does the ownership work? Does a BufferSlice own the Buffer it slices (or share ownership with multiple slices?)?

A buffer Protocol might make sense, but then I think we should do it in a follow up PR

Alternatively, we could have Buffer use BufferSlice so that BufferSlice is the only buffer object used in the rest of cudf.
In any case, I think we should wait until SpillableBuffer also uses BufferSlice and TenableBuffer so we have a better picture of the exact use cases.

docs/cudf/source/developer_guide/library_design.md

python/cudf/cudf/_lib/column.pyx

python/cudf/cudf/core/buffer/tenable_buffer.py

Co-authored-by: Lawrence Mitchell <[email protected]>

…buffer

vyasr

A few small change requests, but in general this looks like the right direction. Thanks @madsbk! I can see from this PR how you would integrate spilling, but also appreciate you splitting up the work this way to make the changes incrementally.

python/cudf/cudf/_lib/column.pyx

python/cudf/cudf/core/buffer/exposure_tracked_buffer.py

vyasr · 2023-06-22T20:45:00Z

python/cudf/cudf/core/buffer/exposure_tracked_buffer.py

+        if exposed:
+            raise ValueError("cannot created exposed host memory")
+        return cast(
+            BufferSlice, ExposureTrackedBuffer._from_host_memory(data)[:]


Is the eventual plan for Buffer.getitem to return a BufferSlice? If not, it might be cleaner to override the method in ExposureTrackedBuffer. I know the whole point of the _getitem/__getitem__ split is to help share some functionality, but the typing confusion here indicates that there are potentially incorrect types that could result from that approach (obviously we can coerce the code into behaving correctly, but it makes it much harder to write intrinsically type-safe code if the type annotations aren't sufficiently valid).

I agree, this is a very valid point!

I think the clean design is to make cudf always work on BufferSlice even when COW and spilling is disabled. Then we get a clean class hierarchy:

COW & Spilling disable:
BufferSlice -> Buffer

COW enabled:
BufferSlice -> ExposureTrackedBuffer -> Buffer

Spilling enabled (when is has been unified with COW in a follow-up PR):
BufferSlice -> SpillableBuffer -> ExposureTrackedBuffer -> Buffer

The downside is that this approach is a bit more intrusive in the default case where COW and spilling is disabled. I think it is worth it but what do you guys think?

cc. @wence- @shwina

I agree that it is probably worth it. I would stage that as work to be done after the COW and spilling unification is complete and we can reevaluate in the context of a cleaner architecture.

python/cudf/cudf/tests/test_copying.py

Co-authored-by: Vyas Ramasubramani <[email protected]>

…buffer

wence-

Very minor typo fix, thanks!

docs/cudf/source/developer_guide/library_design.md

wence- · 2023-06-30T14:04:48Z

python/cudf/cudf/core/buffer/buffer.py

            "shape": (self.size,),
            "strides": None,
            "typestr": "|u1",
            "version": 0,
        }

-    def get_ptr(self, *, mode) -> int:
+    def get_ptr(self, *, mode: Literal["read", "write"]) -> int:


No need to change this, but just to note that Literal doesn't get handled very well by type-checkers if the argument comes from a variable, rather than a literal value (unless the variable is marked with : Final). Since they don't do dataflow analysis

https://mypy-play.net/?mypy=latest&python=3.11&gist=6541ee12e80daeb4b0837563d98dc442

Co-authored-by: Lawrence Mitchell <[email protected]>

madsbk · 2023-07-03T19:15:32Z

/merge

madsbk · 2023-07-03T19:16:07Z

Thanks all for the reviews

@wence-

…13801) This PR de-couples buffer slices/views from owning buffers. As it is now, all buffer classes (`ExposureTrackedBuffer`, `BufferSlice`, `SpillableBuffer`, `SpillableBufferSlice`) inherent from `Buffer`, however they are not Liskov substitutable as pointed by @wence- and @vyasr ([here](#13307 (comment)) and [here](#13307 (comment))). To fix this, we now have a `Buffer` and a `BufferOwner` class. We still use the `Buffer` throughout cuDF but it now points to an `BufferOwner`. We have the following class hierarchy: ``` ExposureTrackedBufferOwner -> BufferOwner SpillableBufferOwner -> BufferOwner ExposureTrackedBuffer -> Buffer SpillableBuffer -> Buffer ``` With the following relationship: ``` Buffer -> BufferOwner ExposureTrackedBuffer -> ExposureTrackedBufferOwner SpillableBuffer -> SpillableBufferOwner ``` #### Unify COW and Spilling In a follow-up PR, the spilling buffer classes will inherent from the exposure tracked buffer classes so we get the following hierarchy: ``` SpillableBufferOwner -> ExposureTrackedBufferOwner -> BufferOwner SpillableBuffer -> ExposureTrackedBuffer -> Buffer ``` Authors: - Mads R. B. Kristensen (https://github.com/madsbk) Approvers: - Lawrence Mitchell (https://github.com/wence-) - Vyas Ramasubramani (https://github.com/vyasr) URL: #13801

madsbk added 2 - In Progress Currently a work in progress improvement Improvement / enhancement to an existing function non-breaking Non-breaking change labels May 8, 2023

github-actions bot added the Python Affects Python cuDF API. label May 8, 2023

madsbk force-pushed the tenable_buffer branch from 11de74c to c2269eb Compare May 9, 2023 09:23

madsbk added 6 commits May 9, 2023 16:38

Impl. TenableBuffer

09b22ab

doc

b43c8cc

remove _get_cuda_array_interface

d27bb25

removed get_raw_ptr

38e3c97

as_tenable_buffer(): accept subclass

f091326

cleanup

b5027ff

madsbk force-pushed the tenable_buffer branch from e56a3b6 to b5027ff Compare May 9, 2023 15:15

fix TenableBuffer check

13e3d2a

madsbk marked this pull request as ready for review May 9, 2023 16:07

madsbk requested a review from a team as a code owner May 9, 2023 16:07

madsbk requested review from wence- and galipremsagar May 9, 2023 16:07

madsbk removed the 2 - In Progress Currently a work in progress label May 9, 2023

madsbk added 3 commits May 10, 2023 08:34

doc

0848ab8

clean up

a924926

doc

122bc70

wence- reviewed May 10, 2023

View reviewed changes

madsbk and others added 8 commits May 10, 2023 15:30

spelling and typos

0debdcd

Co-authored-by: Lawrence Mitchell <[email protected]>

get_ptr(): make mode mandatory

e7a44ac

remove BufferSlice.exposed

d9fc54e

from_column_view(): mark_exposed when the relationship isn't known

a9c320e

Merge branch 'branch-23.06' of github.com:rapidsai/cudf into tenable_…

8e26ee2

…buffer

doc

3b63e9d

cleanup

1c13953

get_ptr: typing mode literal

d1b49ee

madsbk added 4 commits May 11, 2023 13:00

Merge branch 'branch-23.06' of github.com:rapidsai/cudf into tenable_…

3964c1f

…buffer

Merge branch 'branch-23.06' of github.com:rapidsai/cudf into tenable_…

350b95d

…buffer

use Self

38b4ba4

Merge branch 'branch-23.06' of github.com:rapidsai/cudf into tenable_…

df53860

…buffer

madsbk requested a review from wence- May 15, 2023 09:59

shwina assigned madsbk May 18, 2023

madsbk changed the base branch from branch-23.06 to branch-23.08 June 19, 2023 07:17

madsbk added 2 commits June 19, 2023 09:22

Merge branch 'branch-23.06' of github.com:rapidsai/cudf into tenable_…

5ec77f5

…buffer

Merge branch 'branch-23.08' of github.com:rapidsai/cudf into tenable_…

1b67bd8

…buffer

madsbk changed the title ~~Tenable Buffer (first step towards unifying copy-on-write and spilling)~~ Exposure Tracked Buffer (first step towards unifying copy-on-write and spilling) Jun 19, 2023

madsbk added 2 commits June 19, 2023 10:42

rename: tenable -> exposure tracked

3b5f56e

doc

2056c6f

vyasr requested changes Jun 22, 2023

View reviewed changes

spelling

8a8fd93

Co-authored-by: Vyas Ramasubramani <[email protected]>

madsbk requested a review from vyasr June 26, 2023 06:40

vyasr mentioned this pull request Jun 29, 2023

Add pylibcudf subpackage with gather implementation #13562

Merged

3 tasks

vyasr approved these changes Jun 29, 2023

View reviewed changes

madsbk added 3 commits June 30, 2023 08:38

added a TODO

f30e11f

Merge branch 'branch-23.08' of github.com:rapidsai/cudf into tenable_…

98581a8

…buffer

Merge branch 'tenable_buffer' of github.com:madsbk/cudf into tenable_…

e4713db

…buffer

wence- approved these changes Jun 30, 2023

View reviewed changes

madsbk and others added 2 commits July 3, 2023 08:42

typo

e21b66b

Co-authored-by: Lawrence Mitchell <[email protected]>

Merge branch 'branch-23.08' into tenable_buffer

c020dbd

rapids-bot bot merged commit d078cff into rapidsai:branch-23.08 Jul 3, 2023

madsbk deleted the tenable_buffer branch July 31, 2023 06:22

madsbk mentioned this pull request Aug 2, 2023

Refactoring of Buffers (last step towards unifying COW and Spilling) #13801

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Exposure Tracked Buffer (first step towards unifying copy-on-write and spilling) #13307

Exposure Tracked Buffer (first step towards unifying copy-on-write and spilling) #13307

madsbk commented May 8, 2023 •

edited

Loading

wence- left a comment

wence- May 10, 2023

madsbk May 10, 2023

madsbk May 10, 2023

wence- May 10, 2023

wence- May 10, 2023

madsbk May 10, 2023

madsbk May 11, 2023 •

edited

Loading

vyasr left a comment

vyasr Jun 22, 2023

madsbk Jun 23, 2023

vyasr Jun 29, 2023

wence- left a comment

wence- Jun 30, 2023

madsbk commented Jul 3, 2023

madsbk commented Jul 3, 2023


		`TenableBuffer` is a subclass of the regular `Buffer` that tracks its "expose" status of its underlying memory. We say that the buffer has been exposed if the device pointer (integer or void*) has been accessed outside of cudf, in which case we have no control over knowing if the data is being modified by a third-party. Additionally, `TenableBuffer` also maintains [weak references](https://docs.python.org/3/library/weakref.html) to every existing `BufferSlice` that points to its underlying memory.

		`BufferSlice` is a subclass of `TenableBuffer` that represents a _slice_ of the memory underlying a tenable buffer.

Exposure Tracked Buffer (first step towards unifying copy-on-write and spilling) #13307

Exposure Tracked Buffer (first step towards unifying copy-on-write and spilling) #13307

Conversation

madsbk commented May 8, 2023 • edited Loading

Checklist

wence- left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

madsbk May 11, 2023 • edited Loading

Choose a reason for hiding this comment

vyasr left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wence- left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

madsbk commented Jul 3, 2023

madsbk commented Jul 3, 2023

madsbk commented May 8, 2023 •

edited

Loading

madsbk May 11, 2023 •

edited

Loading