Slim Memory; remove hasByteBuffer() and getByteBuffer() methods

[leventov]

Because of apache/druid#5335 (comment) (see the last part of the message) Memory objects are going to need to carry minimal overhead over the memory bytes themselves, at least, in comparison with ByteBuffer (in the linked message, an example API uses ByteBuffer, because it's what currently used in Druid, but it's really going to be replaced with Memory).

In order to achieve this, I think separate ResourceState objects need to be dropped. Memory is either it's own "resource state" (for Memory objects, obtained from wrap(), allocate(), or map() calls), or references to another Memory as it's "resource state" (for Memory objects, obtained from region() calls). Buffer also references to some Memory as it's resource state.

Memory has the functionality of ResourceState as package-private methods.

StepBooleans should be "inlined" to be just volatile boolean fields. (And isReadOnly shouldn't be volatile, it should be just final, because it's not changing during Memory lifetime.

Heap and off-heap Memory should also probably be separated, because off-heap needs a lot of fields, not needed by on-heap Memory: handle, valid (we change the validity state only when we unmap of deallocate direct memory, so not needed for on-heap), reference to ByteBuffer, etc. Off-heap Memory could be much beefier than on-heap, because we are not going to map or allocate small off-heap Memories.

Because of this, I think hasByteBuffer() method should be removed, because we cannot consistently support it, when wrapping on-heap ByteBuffers. Also, I think this method is useless.

Class hierarchy could look like this:

Memory
|
WritableMemory
|
BaseWritableMemoryImpl------
|                           |
WritableMemoryImpl          NonNativeEndianWritableMemoryImpl
|                           |
OffHeapWritableMemoryImpl   OffHeapNonNativeEndianWritableMemoryImpl

OffHeapWritableMemoryImpl and OffHeapNonNativeEndianWritableMemoryImpl have pretty much the same bodies, but I don't see how to avoid this repetition.

[leerho]

You are recommending a lot of changes here, I will have to think through them separately.

I think hasByteBuffer() method should be removed, because we cannot consistently support it, when wrapping on-heap ByteBuffers.

I'm not sure what you mean by "cannot consistently support". If we have wrapped a ByteBuffer we MUST retain a pointer to it so that it doesn't go out of scope before we are done with it. That being the case, providing the hasByteBuffer() is merely a convenience so that the user isn't surprised when getByteBuffer() returns null. It parallels the ByteBuffer.hasArray() method.

Also, your discussion is about reducing memory footprint. The hasByteBuffer() method has no impact on memory footprint.

[leventov]

I'm not sure what you mean by "cannot consistently support". Also, your discussion is about reducing memory footprint. The hasByteBuffer() method has no impact on memory footprint.

In the course of changes that I suggested, on-heap Memory won't have a ByteBuffer field, so when wrap(ByteBuffer) is called with a heap ByteBuffer, it carves the underlying byte[] object out and doesn't retain a link to the BB. So it won't be able to say, if Memory was created via wrap(BB) or wrap(byte[]).

If we have wrapped a ByteBuffer we MUST retain a pointer to it so that it doesn't go out of scope before we are done with it.

It's true for off-heap, but not on-heap ByteBuffers.

That being the case, providing the hasByteBuffer() is merely a convenience so that the user isn't surprised when getByteBuffer() returns null.

I missed that there is getByteBuffer(). I suggest to remove it too.

[leerho]

I created ResourceState to simplify the code both in terms of understanding, maintenance, and vastly fewer parameters being passed around. The on-heap case requires, I think, 10 parameters. The off-heap case adds 6 more.

I am a little puzzled by your concern about space consumed by Memory when wrapping on-heap arrays. I thought that the vast majority of use of ByteBuffer in Druid was for accessing data off-heap. Do you use also have heavy use of on-heap ByteBuffers?

---

WRT the Read-only flag.
Yes, in theory, for the on-heap case, it could be final and passed to the constructor ... along with the unsafeObj_, unsafeObjHeader_, capacity_, cumBaseOffset_, memReqSvr_, valid_, regionOffset_, byteBuf_, resourceOrder_. The off-heap case would add nativeBaseOffset_, handle_, file_, fileOffset_, raf_, mbb_.

---

Responding to your next message ... why remove getByteBuffer()? It is there for the same reason that ByteBuffer has .array(). The user may want to use Memory as a more flexible tool for manipulating data. So you wrap(BB), pass Memory to some worker, then when you get the Memory back, you can recover the modified BB. OR, if the recipient is downstream from the worker. Otherwise you would have to pass both the BB and the Memory objects along.

[leerho]

I am not opposed to your proposed hierarchy, but I think eliminating ResourceState will be more complex than you think.

[leventov]

I am a little puzzled by your concern about space consumed by Memory when wrapping on-heap arrays. I thought that the vast majority of use of ByteBuffer in Druid was for accessing data off-heap. Do you use also have heavy use of on-heap ByteBuffers?

I the first message of this thread, I linked specifically to the place, where we are going to need to allocate a lot of really small on-heap Memory objects (starting from 0-8 bytes in payload).

The user may want to use Memory as a more flexible tool for manipulating data. So you wrap(BB), pass Memory to some worker, then when you get the Memory back, you can recover the modified BB.

I thought about this. When you wrap(BB), you already have a BB. So the pattern could be

mem = wrap(BB)
doSomethingWith(mem)
use(BB); // BB is modified

This is a pretty common pattern.

[leerho]

And if the recipient is downstream?

[leventov]

Then you could pass them both downstream: doSomethingWith(mem, BB).

[leerho]

Seems kind of silly if mem wraps BB, why you would have to pass both. Nonetheless, we are arguing over just one of the fields that is just a reference. Have you looked closely at all the others?

[leventov]

What about the other fields? On-heap Memory really needs only the following fields:

• Object unsafeObj, long cumBaseOffset, long capacity
• boolean isReadOnly
• ByteBuffer buf (under debate)

Off-heap additionally needs a reference to a cleaner, mandatory reference to a BB, boolean isValid, etc.

[leerho]

• unsafeObjHeader_ OK, we could eliminate this one and reference UnsafeUtil.

• valid_ OK, for on-heap, this would be an internal static and not need to be passed.

• memReqSvr_ This is required so that a recipient can request more memory from the memory owner. This does happen and should not depend on whether the memory is on-heap or off-heap, as the recipient does not know and should not care. If Druid were to actually take advantage of this, it would reclaim a whole lot of wasted memory today. Our off-heap sketches today already can utilize this.

• regionOffset_ Also required for regions within on-heap allocated spaces. ByteBuffer has a similar field. Note that even for on-heap, this is different from cumBaseOffset, which includes the unsafeObjHeader_

• resourceOrder_ Required. We should not assume the byte order of fields within a wrapped byte[]. Those bytes could come from anywhere.

• byteBuf_ (I still think we should offer this.) also for symmetry with off-heap.

[leventov]

memReqSvr_ - If Druid were to actually take advantage of this, it would reclaim a whole lot of wasted memory today.

I don't see MemoryRequestServer and MemoryManager API as really usable. If you need some kind of pooling or special resource management, the requirements tend to be quite specific. And you don't really need this to be inside the memory object, you could always pass a reference to the pool along with the reference to the memory, when required (and it happens really rarely.)

regionOffset_ - I see getRegionOffset() method now, but what't an application of it? Let's remove this method too. Also, you could implement this method without the field as

return cumBaseOffset - (unsafeObj == null ? 0 : unsafe.arrayBaseOffset(unsafeObj.getClass()));

resourceOrder_ - class hierarchy helps, so resource order information is extractable from the class of the Memory :)