WritableMemoryDirectHandler allows only one allocate() call

[jmalkin]

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.

[leerho]

blockquote, div.yahoo_quoted { margin-left: 0 !important; border-left:1px #715FFA solid !important; padding-left:1ex !important; background-color:white !important; } The handler implements MemoryRequest and owns the responsibility of allocating memory.  'So the method that returns the new memory should have assigned "this" to the new memory object before returning it. This is obviously a bug.

Sent from Yahoo Mail for iPhone

On Wednesday, April 12, 2017, 5:34 AM, Jon Malkin notifications@github.com wrote:

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

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub, or mute the thread.

[jmalkin]

That's what we decided was likely the case, but we weren't sure how to get the relevant ResourceState object, nor were we confident enough we understood how to use it even if we had a way to acquire it.

Maybe something like this?

  @Override
  public WritableMemory request(final long capacityBytes) {
    // default allocate on heap
    final WritableMemory newMem = WritableMemoryImpl.allocate((int)capacityBytes);
    ((WritableMemoryImpl) newMem).state.putMemoryRequest(this);
    return newMem;
  }

I don't think we'd free any memory in here; the auto-closeable would handle that, I'd hope. But I'm not that comfortable with the case to WritableMemoryImpl. It also doesn't seem to allow for a way to request additional off-heap memory.

Anyway, if this is a sane way to do things I can try to make a PR around this change.

[jmalkin]

That change allows the unit test I'd been failing to pass, but I'm still not entirely sure it's correct.

[leerho]

blockquote, div.yahoo_quoted { margin-left: 0 !important; border-left:1px #715FFA solid !important; padding-left:1ex !important; background-color:white !important; } I'm in a crowded subway in Tokyo,  if I have a chance tonight I'll try to look at the code. It is a little challenging with just my cellphone. :)

Sent from Yahoo Mail for iPhone

On Wednesday, April 12, 2017, 2:08 PM, Jon Malkin notifications@github.com wrote:

That change allows the unit test I'd been failing to pass, but I'm still not entirely sure it's correct.

— You are receiving this because you commented.

Reply to this email directly, view it on GitHub, or mute the thread.

[jmalkin]

I was not expecting immediate replies while you're traveling! Creating an issue was so that there'd be a convenient reference point when you're back :)

[leerho]

The original bug has been fixed. In addition, the Memory Request / mechanism has been redesigned with a MemoryManager class that can be extended by the user.