jvanstraten commented on a change in pull request #12116:
URL: https://github.com/apache/arrow/pull/12116#discussion_r805894493
##########
File path: cpp/src/arrow/device.h
##########
@@ -223,4 +236,53 @@ class ARROW_EXPORT CPUMemoryManager : public MemoryManager
{
ARROW_EXPORT
std::shared_ptr<MemoryManager> default_cpu_memory_manager();
+/// A memory manager that uses the immutable zeros interface of the given
memory pool,
+/// rather than the normal mutable buffer interface.
+class ARROW_EXPORT CPUImmutableZerosMemoryManager : public MemoryManager {
Review comment:
> Couldn't this be placed inside device.cc instead?
Probably, since it's not used extensively or at all yet. But it seems odd to
me to make `CPUMemoryManager` public (and it needs to be, there is code
referring to it directly) while hiding its cousin. IMO it should be public for
symmetry if nothing else.
> Additionally, I don't see `is_mutable` overridden.
Oops, I completely forgot to do that. 41ebefc
> [...] is it useful to have?
My logic here is that if a buffer is associated with a memory pool that
generates immutable buffers of which the data corresponds to some pattern
(otherwise the buffers wouldn't be very useful in general), one should be able
to assume that the contents of this buffer actually correspond to said pattern.
If however views/copies of other buffers can be created within each CPU memory
pool, which the previous implementation did (in fact it assumed there is only
one CPU memory pool, as it always just used AllocateBuffer for the default CPU
memory pool) that invariant does not hold. The `is_mutable()` flag was my way
of solving this, or at least my way of catching as many possible application
logic problems related to this as possible (a user can just make their own
memory pools that ignore the destination memory pool argument in `CopyBufferTo`
and `ViewBufferTo`, not unlike what the default CPU memory pool was doing, so
it's certainly not foolproof).
However, whether this logic is sound and/or whether this is desirable is
certainly disputable. As is the name of the method.
> We could also check `Buffer::is_mutable` in `CopyBufferTo` instead above.
That would be too restrictive per the above reasoning. For example,
`MemoryManager::is_mutable()` is/should be checked whenever a view is made of
another buffer within the confines of that memory manager; if the memory
manager makes immutable buffers and thus is making some assertion about the
contents of its buffers, making the view is denied (that's pessimistic, because
the data *might* still conform to the pattern, but at this point the user is
probably doing something they didn't intend to do regardless of what's actually
in the buffer). On the other hand, if the memory pool can make mutable buffers
and thus does not make any assertions about buffer contents, it's perfectly
fine to return a view of an existing buffer, regardless of that buffer's
mutability.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
