Ds java issue358 #136
Ds java issue358 #136
Conversation
an issue in Memory, and the Druid issue #11544.
There was a problem hiding this comment.
I have scanned through this PR and did not notice any unusual changes, but did not immediately understand some of the more subtle changes as you mentioned in the description.
Concerning the original issue raised on ds-java, I would like to understand how these changes address the problem. It looks as if a memory request server is now enforced via the default, (or provided from the user) for dynamic backed memory objects, such as byte buffers as is the case with Druid. Does this allow the memory to expand without the null-pointer exception - assuming the null pointer was related to not having a memory request server available?
| * Gets the MemoryRequestServer object used by dynamic Memory-backed objects | ||
| * to request additional memory. To customize the actions of the MemoryRequestServer, | ||
| * extend the MemoryRequestServer interfact and | ||
| * set using {@link WritableMemory#allocateDirect(long, ByteOrder, MemoryRequestServer)}. |
There was a problem hiding this comment.
Minor: typo in interfact.
There was a problem hiding this comment.
The original design provided the MemoryRequestServer callback only for Memory segments allocated via WritableMemory.allocateDirect(...) calls. Memory segments allocated via WritableMemory.wrap(ByteBuffer) did not have this capability. This was a major oversight since all off-heap memory in Druid is allocated using ByteBuffers! It is unusual that no one has uncovered this until now. Nonetheless, the fix involves instrumenting all the paths involved in providing this callback mechanism for wrapped ByteBuffers.
| */ | ||
| public static final long getCurrentDirectMemoryMapAllocated() { | ||
| return BaseStateImpl.currentDirectMemoryMapAllocated_.get(); | ||
| } |
There was a problem hiding this comment.
What is the difference between allocated vs allocations? Is the latter some kind of rate measurement?
There was a problem hiding this comment.
Allocated: the amount of total off-heap memory allocated via the WritableMemory.allocateDirect(..) mechanism.
Allocations: The number of allocation requests.
| memNNO.putShort(0, (short) 1); | ||
| assertNull(ReflectUtil.getUnsafeObject(memNNO)); | ||
| assertTrue(memNNO.isDirect()); | ||
| checkCombinations(memNNO, off, cap, memNNO.isDirect(), NNO, false, true); |
There was a problem hiding this comment.
Is the result of ReflectUtil.getUnsafeObject null in both cases because the memory is off-heap?
|
Does this allow the memory to expand without the null-pointer exception -
assuming the null pointer was related to not having a memory request server
available?
Yes. The default allows one such request, which places the new Memory on
the heap. If the user wants a more sophisticated allocation mechanism they
need to provide their own extension of MemoryRequestServer.
…On Tue, Aug 10, 2021 at 7:58 AM David Cromberge ***@***.***> wrote:
***@***.**** approved this pull request.
I have scanned through this PR and did not notice any unusual changes, but
did not immediately understand some of the more subtle changes as you
mentioned in the description.
Concerning the original issue raised on ds-java, I would like to
understand how these changes address the problem. It looks as if a memory
request server is now enforced via the default, (or provided from the user)
for dynamic backed memory objects, such as byte buffers as is the case with
Druid. Does this allow the memory to expand without the null-pointer
exception - assuming the null pointer was related to not having a memory
request server available?
------------------------------
In
datasketches-memory-java8/src/main/java/org/apache/datasketches/memory/WritableBuffer.java
<#136 (comment)>
:
> @@ -363,10 +366,11 @@ void putBooleanArray(boolean[] srcArray, int srcOffsetBooleans,
//OTHER WRITABLE API METHODS
/**
- * For Direct Memory only. Other types of backing resources will return null.
- * Gets the MemoryRequestServer object used by dynamic off-heap (Direct) memory objects
- * to request additional memory.
- * Set using ***@***.*** WritableMemory#allocateDirect(long, MemoryRequestServer)}.
+ * For ByteBuffer and Direct Memory backed resources only. Heap and Map backed resources will return null.
+ * Gets the MemoryRequestServer object used by dynamic Memory-backed objects
+ * to request additional memory. To customize the actions of the MemoryRequestServer,
+ * extend the MemoryRequestServer interfact and
+ * set using ***@***.*** WritableMemory#allocateDirect(long, ByteOrder, MemoryRequestServer)}.
Minor: typo in interfact.
------------------------------
In
datasketches-memory-java8/src/main/java/org/apache/datasketches/memory/internal/BaseStateImpl.java
<#136 (comment)>
:
> +
+ /**
+ * Gets the current number of active direct memory map allocations.
+ * @return the current number of active direct memory map allocations.
+ */
+ public static final long getCurrentDirectMemoryMapAllocations() {
+ return BaseStateImpl.currentDirectMemoryMapAllocations_.get();
+ }
+
+ /**
+ * Gets the current size of active direct memory map allocated.
+ * @return the current size of active direct memory map allocated.
+ */
+ public static final long getCurrentDirectMemoryMapAllocated() {
+ return BaseStateImpl.currentDirectMemoryMapAllocated_.get();
+ }
What is the difference between allocated vs allocations? Is the latter
some kind of rate measurement?
------------------------------
In
datasketches-memory-java8-tests/src/test/java/org/apache/datasketches/memory/test/LeafImplTest.java
<#136 (comment)>
:
> - checkDirect(memLE, off, cap);
+ // Off Heap, Native order, No ByteBuffer, has MemReqSvr
+ try (WritableHandle wdh = WritableMemory.allocateDirect(cap, NO, dummyMemReqSvr)) {
+ WritableMemory memNO = wdh.getWritable();
+ memNO.putShort(0, (short) 1);
+ assertNull(ReflectUtil.getUnsafeObject(memNO));
+ assertTrue(memNO.isDirect());
+ checkCombinations(memNO, off, cap, memNO.isDirect(), NO, false, true);
+ }
+ // Off Heap, Non Native order, No ByteBuffer, has MemReqSvr
+ try (WritableHandle wdh = WritableMemory.allocateDirect(cap, NNO, dummyMemReqSvr)) {
+ WritableMemory memNNO = wdh.getWritable();
+ memNNO.putShort(0, (short) 1);
+ assertNull(ReflectUtil.getUnsafeObject(memNNO));
+ assertTrue(memNNO.isDirect());
+ checkCombinations(memNNO, off, cap, memNNO.isDirect(), NNO, false, true);
Is the result of ReflectUtil.getUnsafeObject null in both cases because
the memory is off-heap?
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#136 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ADCXRQT2K3X4FORRDYGJ7L3T4E5BTANCNFSM5B3JVHVQ>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&utm_campaign=notification-email>
.
|
|
Oh, why so? In that case I would think we need a custom request server in our Druid extension. |
|
Yes.
…On Wed, Aug 11, 2021 at 7:19 PM Alexander Saydakov ***@***.***> wrote:
Oh, why so? In that case I would think we need a custom request server in
our Druid extension.
—
You are receiving this because you modified the open/close state.
Reply to this email directly, view it on GitHub
<#136 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ADCXRQSIO6W7Y2X7CBAWKHLT4MVTHANCNFSM5B3JVHVQ>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&utm_campaign=notification-email>
.
|
This PR fixes the datasketches-java issue #358, which was actually an issue in Memory, and the Druid issue #11544.
This is a rather complex set of changes that involved about 30 classes. Along the way I found some other other discrepancies and missing capabilities in the API and fixed those as well. The tests have been extensively rewritten to cover (I hope) all cases. I do want to add one more test that closely reproduces the bug discovered by the Druid folks.
I don't expect anyone to understand all the subtleties in these changes, at least not by only scanning. Nonetheless, I would appreciate another pair of eyes, just to see if anything looks weird.
There will be a few more PRs coming, but with different focus.