Buffer / Memory consistency

[leerho]

Currently Buffer does not have a wrap(byte[] ... BO) parallel to Memory. Do you need this?

Another thought I had was to eliminate all the static factory methods in Buffer and WritableBuffer and require that the user always create a Buffer from Memory. This would make your suggestion of keeping a Memory reference in Buffer make even more sense and would simplify the code and tests quite a bit. Thoughts? (Sketches don't use Buffer so this is fine with me.)

[leventov]

Another thought I had was to eliminate all the static factory methods in Buffer and WritableBuffer and require that the user always create a Buffer from Memory.

I didn't understand this, do you mean to make constructors public?

This would make your suggestion of keeping a Memory reference in Buffer make even more sense and would simplify the code and tests quite a bit. Thoughts?

didn't understand this too.

[leerho]

For example, consider eliminating the Buffer Primitive Heap Methods.

Note that the origin Memory is now saved in WritableBufferImpl here and returned here.

So the equivalent user code for creating a Buffer from a byte[] would be

Memory mem = Memory.wrap(byte[], ...); Buffer buf = mem.asBuffer();

The same goes for all the other primitive array types, and for WritableBuffer.

It would be a little slower to create the Buffer, but returning to Memory would be super quick. (This was your suggestion!). Plus it would eliminate a bunch of code from Buffer, and WritableBuffer.

The only static method that we probably have to retain would be wrap(BB).

[leventov]

Ok, I see. I don't know, if I'm going to need those methods in Druid or not. Couldn't answer yet. Let's not remove them yet.

BTW here: https://github.com/DataSketches/memory/blob/2a58049ce583ad2902bb4ebbc5d332bd598f5d28/src/main/java/com/yahoo/memory/WritableBufferImpl.java#L120-L124

I think should memoize the created WritableMemoryImpl instance in the same field. And rename asMemory() to getMemory().

[leerho]

I really like the idea of removing these static wrap (factory) methods from Buffer and WritableBuffer.

It is not preventing you from doing these methods, it just would be a tad bit slower. It would make it easier to ensure that whatever factory creation you can do with Memory you can do with Buffer. It would become a one-liner:

Buffer buf = Memory.wrap(...).asBuffer(); //OR Writable wbuf = WritableMemory.wrap(...).asWritableBuffer();

It also would make creating your NonNativeWritable*Impl classes a whole lot less tedious.

If you discover that the speed of instantiating these Buffers is a problem, we could add selected ones back.

[leventov]

You was talking about Buffer.wrap() before. Now about Memory.wrap(). I don't understand.

[leerho]

It was a typo. I corrected it. I am talking about Buffer and WritableBuffer.

[leventov]

Not sure how does it affect tediousness of creation of NonNativeWritable*Impl but sounds ok to me. I used only Buffer.wrap(ByteBuffer) factory yet, but not the ones accepting arrays.

[leerho]

If you could respond to the copyTo overlap issue and the stateful information issue, I will fix those and check into master. This removing of these wrap methods I want to do in a separate branch.

[leerho]

Please look at removeBufferStatics. Also did some simplification of these methods in Memory as well.