I suggest to make those utility classes package-private to minimize API.

31-multiplication chain algorithm, used in String and ByteBuffer, is slow and doesn't distribute the values well. A faster algorithm could be used, such as xxHash.

Currently when asBuffer() is called it keeps a copy of the calling WritableMemoryImpl in the target WritableBufferImpl as a BaseWritableMemoryImpl originMemory. This seems a bit odd.

• I think this field should be moved to BaseWritableBufferImpl.

• Also, why don't we also do the reverse? If asMemory is called from a Buffer, keep a reference to the Buffer in the Memory impl. This would provide very fast switching back and forth between Buffer and Memory and it would allow us to focus Buffer strictly on the positional methods only, significantly reducing its API.

Thoughts?

@leerho you mentioned that you have seen performance regression of Memory after the introduction of NonNative impls. Let's track this issue here. Could you please specify the exact commits between which you have seen it?

When off + cap is more than the file size, the file should not be extended regardless of the map mode (read-only or read-write) and the read-only status of the file. Instead, an exception should be thrown. This behaviour should be specified in the javadocs of those methods.

As a user of the Memory API, I was surprised not to find such a method. Spent several minutes before I found Memory.copyTo(WritableMemory).

I suggest to add a method WritableMemory.putBytes(long, Memory, long, long), that delegates to Memory.copyTo().

Using the default direct memory handler, an application can make only a single call to allocate(), because the WritableMemory object is never associated with a handler:
https://github.com/DataSketches/memory/blob/master/src/main/java/com/yahoo/memory/WritableMemoryDirectHandler.java#L57-L59

As an example of trying to increase the size of some buffer, say I have this simple method:

private WritableMemory growBuffer(WritableMemory srcMem, int currentSize, int newSize) {
  MemoryRequest mr = mem.getMemoryRequest();
  if (mr == null) { throw new Exception(); }
  WritableMemory newMem = memoryRequest.request(newSize);
  srcMem.copyTo(0, newMem, 0, currentSize);
  return newMem;
}

If I replace the old memory reference with the new one (and it would be strange to need to hold on to the original Memory object for the life of the object), things work as expected on the first call to growBuffer() but on the next call, mem.getMemoryRequest() will return null.

isResourceReadOnly() - method doesn't make much sense, if not to say that it's misleading and dangerous -- it's very easy to think that this method designates the Memory's or Buffer's read-onlyness, but it doesn't. See #69.

I suggest to remove this method and replace it with isReadOnly(), that returns localReadOnly instead of the ResourceState's read-only.

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.)

Why do we need duplicate code here and here when we could make these static methods that could also be leveraged by other static methods that do the same checks, e.g., here?

I would suggest consolidating all these check and assert methods together in one place, like Util.

Java 8 is long in the tooth. Newer versions will pose challenges for this library in particular, but the project risks being left behind over time due to incompatibility with modern JDKs.

I cannot find a method to check that two ranges of bytes in two Memories are equal. compareTo() is inefficient because it compares data byte by byte.

I don't see the point in this repetition: WritableMemory.writableMap(). Yes, simple map() clashes with Memory.map(), but you could say the same about all wrap() methods as well.

@leventov

This is more of a comment than a real issue, but here FYI.

getCharsFromUtf8() originally returned void. But understanding how many characters were actually decoded can be valuable to know, especially if the user needs to increment a pointer into the destination Appendable. But it is not possible for the method to know how many characters already existed in the destination before appending.

I have updated the method to return an integer of the number of characters decoded in the current PR.

The asymmetry is that putCharsToUtf8() returns the position in the WritableMemory after the last byte has been written from the encoding process instead of the number of bytes encoded.

A question for you is should we change the putCharsToUtf8() to return the number of bytes encoded? This would make the two methods more symmetric.

We had no tests for the case where the destination Appendable already had some content so I added a test for that.

Currently when using this library I get an error "JDK must be either 7 or 8".

https://github.com/DataSketches/memory/blob/5356a8b1db8aaf985acb22766e803f8737de115e/src/main/java/com/yahoo/memory/AccessByteBuffer.java#L32-L80

Why read-only and writable branches are so different?

Since Memory is not ByteBuffers, it's not covered by JMX statistics of direct byte buffer allocation. It's viable to add at least basic atomic counters, like total direct memory allocated, total direct memory mapped. I suggest to expose them as static methods in Memory: Memory.totalDirectMemoryAllocated(), etc.

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.

Previously I turned this idea down, but now I've given up the idea to make all libraries to be compatible with Memory from the bottom up (like RoaringBitmap). Those libraries, if they don't currently use unsafe or any other ugly hacks, just vanilla ByteBuffer API, won't agree to migrate to Memory unless it supports Java 9, now Java 10, and Unsafe-free mode of operation. Also non-OpenJDK operation (e. g. Eclipse OpenJ9, that is famous to break often when people try to access private fields of some impls of OpenJDK's classes.) Adding support of all those things would be very very hard in Memory, and a lot of work.

Previously I was concerned that wrapping direct memory as DirectByteBuffer is unsafe, because if the original memory is freed, the buffer could still be used. Well, yes, it's unsafe (we could even call those methods like unsafeByteBufferView()), but there is no other way. Those methods should be used only in specific situations carefully. Also we could make it a little less dangerous, if we install a reference to the Memory object into ByteBuffer's att field.

@leventov

I made some style and cosmetic changes to your PR which I pulled into the utf8 branch here.
I had to add finals, make some methods static that could have been, etc. tools/SketchesCheckstyle should have caught those.

Question 1: You did not create a parallel get/put utf8 for buffer. Why?

Question 2: Have you done any testing that shows that adding this utf8 capability into Memory improves performance over what could have been done external to Memory?

Creating this issue for myself - to not forget to look into this

E. g. here:

https://github.com/DataSketches/memory/blob/8034c6c183bd0a4d92035a9b14564f4b6a17bda7/src/test/java/com/yahoo/memory/WritableMemoryTest.java#L30-L32

The test depends on the platform byte order, that is probably not so good.

Also, it would be nice to add testing on a big-endian platform, is some CI service that is free for OSS software supports it (Travis doesn't yet).

Memory.hashCode(offset, lenBytes)

I'm not sure to what extent we want to support platforms that don't support unaligned memory access, such as old ARMs, SPARC, etc. Currently an attempt to do unaligned reads and writes with Memory will lead to SIGBUS crash, that is worse than throwing an exception.

jdk.interal.misc.Unsafe includes intrinsics for doing unaligned reads and writes, but sun.misc.Unsafe unfortunately doesn't.

It's essential when app does direct memory allocations via both ByteBuffers and Memory, to let ByteBuffer's allocation path "know" that there are not enough direct memory, if it's allocated via Memory, and call System.gc() if needed, etc. See the code of Bits.java.

Memory.copyToWritableMemory()
Memory.copyToWritableMemory(offset, lenBytes)
Memory.toBytes()
Memory.toBytes(offset, lenBytes)

The similarity of those methods and easiness of confusion mandates that there shouldn't be a separate equalTo() method, it should just override equals(). I. e. the same as in ByteBuffer.

From it's javadoc, it returns the modified value, hence addAndGet, not getAndAdd.

https://github.com/DataSketches/memory/blob/5356a8b1db8aaf985acb22766e803f8737de115e/src/main/java/com/yahoo/memory/WritableMemoryImpl.java#L110-L112

I don't understand why this is needed, what relation ByteBuffer's position and limit has to Memory, based on it.

#46 (comment)

I think we need to do those two things for safety. They provide shallow (because under assert) runtime checks that Memory is not being cast to WritableMemory and used as it.

These threads are a little hard to find, but they relate to the same issue

• Fixed Bugs in test

• Merge pull request #50 from DataSketches/get-and-add-fixes

This issue relates to the larger task of slimming ResourceState especially for on-heap: #42

@leventov

Last year you stated an objective of not having stateful information in Memory or Buffer (or BaseBuffer). This leaves open the possibility of creating implementations that are independent of Unsafe.

By adding ResourceState to BaseBuffer, this is now broken. But it is not really needed there. Shall I move it back to WritableBufferImpl ?

Once the API stabilizes, we need to go back and see what methods we have in Memory that would also be valuable in Buffer.

Refer to #49

WritableMemory.writableDuplicate() is an indistinguishable object from the original, I think this method should be removed.

Memory should add method(s) for efficient (i. e. without extra data copy, if possible) write of it's contents (or subrange) to a file. WritableByteChannel has method write(ByteBuffer). Hence when we create a large direct Memory (potentially bigger than 2 GB) we should create e. g. 1 GB direct ByteBuffer "blocks" under the hoods anyway, and use them only for such file write operations.

Heap byte[] or ByteBuffer-based memory is OK already. (byte[] will need to wrap into a ByteBuffer before writing, e. g. create a little garbage, but it's unavoidable and OK. ByteBuffer-based it no better, it will need to create duplicate() for write anyway, to avoid concurrency issues with ByteBuffer offsets.)

Non byte[] array based memory (short[], int[] etc.) couldn't be written to a file without an extra data copy one way or another (unless you want to create JNI implementation and deal with file descriptors yourself :) But it's probably OK because Memory that is created this way probably shouldn't often be written to a file directly. At least there is no such need in Druid. Druid specifically is going to need to write only large direct Memory into a file.

1. Why does Memory.copyTo() prohibit overlap of copied regions? Why doesn't it handle it properly? Well, it might be a source of bugs, I agree. Can we add a method copyToMaybeOverlapping()?
2. WritableMemoryImpl.copyTo() checks this == destination, but it should really compare srtParent and dstParent: https://github.com/DataSketches/memory/blob/3878b82529f4e6de531ffbff6b3b60206f27476f/src/main/java/com/yahoo/memory/WritableMemoryImpl.java#L305-L312
   Because the same array could be wrapped two times.
3. They same applies to all bulk array get and put methods, because source or dest array could be the same array that was previously wrapped to produce the Memory object.
4. And the same for Buffer.

Copyright headers in the project say Copyright <year>, Yahoo! Inc. that is not correct, because I didn't sign any CLA with Yahoo.

It's suggested to change all headers to Copyright <year> Memory contributors and add a NOTICE file that lists copyright holders: Yahoo Inc. and Metamarkets Group Inc.

@leventov

Somewhere you mentioned using a different CheckStyle template. Where was the comment? Or what was the recommendation and why?

The usage of direct Memory is currently fundamentally prone to use-after-free, if the direct memory object is garbage-collected at the moment when a some of it's method is called or it is compared with other Memory e. g. in equals(). See Reference.reachabilityFence() Javadoc for details.

Now DirectByteBuffer's code looks like this: http://hg.openjdk.java.net/jdk/jdk/file/a6ede2dabe20/src/java.base/share/classes/java/nio/Direct-X-Buffer.java.template#l269, those try { ... } finally { Reference.reachabilityFence(this); } are in every DirectByteBuffer's method.

Reference.reachabilityFence() method is Java 9+, but informally it's known that in Hotspot JVM any method call with the object as the argument has the same effect as Reference.reachabilityFence(), e. g. Objects.requireNonNull(): http://mail.openjdk.java.net/pipermail/core-libs-dev/2018-February/051312.html

Memory.java#L198, permalink seems incorrect.
It initializes the memory with the offset 0, instead of the supplied offset into the array. I might be unclear on the method's contract, but this seems counterintuitive to what I was expecting - an offset of x with length l as an argument means that the memory would be a window into the [x..x+l] bytes of the provided byte array. This is what ByteBuffer.wrap() does.

Is this the expected behavior of the method or is it a bug?

Thanks!

@leventov

You created two degenerate objects, ZERO_SIZE_MEMORY and ZERO_SIZE_BUFFER, presumably for performance. I normally like this idea, however, I want to point out some subtle unintended consequences that you might not be aware of.

The ResourceState associated with each object contains information about the resource that the object is mapped to and the user can query some of this information via methods like hasByteBuffer(). The degenerate objects you created are specifically zero byte array resources.

Suppose the user's code does something like this:

ByteBuffer bb = ByteBuffer.allocateDirect(0); //accidentally
//.... much later and somewhere else ...
WritableMemory wmem = WritableMemory.wrap(bb);
assert wmem.hasByteBuffer();  //Fails!
assert wmem.isDirect(); //Fails!

This might be very misleading for the user trying to debug why his ByteBuffer derived WritableMemory suddenly lost its mapping to a direct ByteBuffer, not realizing that under the covers we did a switcheroo on him because it had zero capacity.

It would be helpful if you could explain what you thought was the most likely scenario that would impact performance. Do you expect to see lots of zero-sized ByteBuffers or zero-sized arrays that you want to wrap? Does this occur frequently in Druid?

I do suggest we change the name of the current one to ZERO_SIZED_ARRAY_MEMORY to make it really clear how it was created.

I changed the public checkBounds() to checkValidAndBounds().

Nonetheless, do we really need this? Since all of our public methods do this check why would a user need this?

Hi There,

This project is still using an old implementation of LGTM's automated code review, which has now been disabled. To continue using automated code review, and receive checks on your Pull Requests, please install the GitHub App on this repository.

Thanks,
The LGTM Team

@leventov

This has bothered me for a while, and since we are making other API changes, I am thinking we may as well make this change as well...

WritableHandle can not extend Handle because both interfaces define the method get(). But changing the method name in WritableHandle to getWritable(), then it would be possible for WritableHandle to extend Handle allowing simple casting between the two.

This will likely affect a bunch of our code, but I haven't tried it to see how much it affects. Other users would need a warning and documentation of this change.

Here are two images of the before and after Handle Hierarchy:

Thoughts?

@leventov

Would you mind taking a look at this to see if this is the right way to do this?

I am somewhat concerned whether Java 9 will allow us to wrap ByteBuffer using reflection like we are doing now.