-
Notifications
You must be signed in to change notification settings - Fork 4.2k
ARROW-2696: [JAVA] enhance AllocationListener with an onFailedAllocation() call #2133
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 5 commits
d79fd98
e5188a8
6a3fc8c
fc97506
ac0b81f
4f64f96
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -72,7 +72,7 @@ protected BaseAllocator( | |
| this(parentAllocator.listener, parentAllocator, name, initReservation, maxAllocation); | ||
| } | ||
|
|
||
| private BaseAllocator( | ||
| protected BaseAllocator( | ||
| final AllocationListener listener, | ||
| final BaseAllocator parentAllocator, | ||
| final String name, | ||
|
|
@@ -276,7 +276,13 @@ public ArrowBuf buffer(final int initialRequestSize, BufferManager manager) { | |
| : initialRequestSize; | ||
| AllocationOutcome outcome = this.allocateBytes(actualRequestSize); | ||
| if (!outcome.isOk()) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. let's avoid checking twice in the base case.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. done |
||
| throw new OutOfMemoryException(createErrorMsg(this, actualRequestSize, initialRequestSize)); | ||
| if (listener.onFailedAllocation(actualRequestSize, outcome)) { | ||
| // Second try, in case the listener can do something about it | ||
| outcome = this.allocateBytes(actualRequestSize); | ||
| } | ||
| if (!outcome.isOk()) { | ||
| throw new OutOfMemoryException(createErrorMsg(this, actualRequestSize, initialRequestSize)); | ||
| } | ||
| } | ||
|
|
||
| boolean success = false; | ||
|
|
@@ -333,9 +339,18 @@ public BufferAllocator newChildAllocator( | |
| final String name, | ||
| final long initReservation, | ||
| final long maxAllocation) { | ||
| return newChildAllocator(name, this.listener, initReservation, maxAllocation); | ||
| } | ||
|
|
||
| @Override | ||
| public BufferAllocator newChildAllocator( | ||
| final String name, | ||
| final AllocationListener listener, | ||
| final long initReservation, | ||
| final long maxAllocation) { | ||
| assertOpen(); | ||
|
|
||
| final ChildAllocator childAllocator = new ChildAllocator(this, name, initReservation, | ||
| final ChildAllocator childAllocator = new ChildAllocator(listener, this, name, initReservation, | ||
| maxAllocation); | ||
|
|
||
| if (DEBUG) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -222,7 +222,122 @@ public void testRootAllocator_createChildDontClose() throws Exception { | |
| } | ||
| } | ||
|
|
||
| private static void allocateAndFree(final BufferAllocator allocator) { | ||
| // Allocation listener | ||
| // It counts the number of times it has been invoked, and how much memory allocation it has seen | ||
| // When set to 'expand on fail', it attempts to expand the associated allocator's limit | ||
| private static final class TestAllocationListener implements AllocationListener { | ||
| private int numCalls; | ||
| private long totalMem; | ||
| private boolean expandOnFail; | ||
| BufferAllocator expandAlloc; | ||
| long expandLimit; | ||
|
|
||
| TestAllocationListener() { | ||
| this.numCalls = 0; | ||
| this.totalMem = 0; | ||
| this.expandOnFail = false; | ||
| this.expandAlloc = null; | ||
| this.expandLimit = 0; | ||
| } | ||
|
|
||
| @Override | ||
| public void onAllocation(long size) { | ||
| numCalls++; | ||
| totalMem += size; | ||
| } | ||
|
|
||
| @Override | ||
| public boolean onFailedAllocation(long size, Accountant.AllocationOutcome outcome) { | ||
| if (expandOnFail) { | ||
| expandAlloc.setLimit(expandLimit); | ||
| return true; | ||
| } | ||
| return false; | ||
| } | ||
|
|
||
| void setExpandOnFail(BufferAllocator expandAlloc, long expandLimit) { | ||
| this.expandOnFail = true; | ||
| this.expandAlloc = expandAlloc; | ||
| this.expandLimit = expandLimit; | ||
| } | ||
|
|
||
| int getNumCalls() { | ||
| return numCalls; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. you should be able to do this count number of calls monitoring using Jmockit's APIs rather than custom building something.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. True, and in fact I had it like that initially. However, I found I was repeating code again and again in the mocks, and it was much easier to just build a custom listener.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ack |
||
| } | ||
|
|
||
| long getTotalMem() { | ||
| return totalMem; | ||
| } | ||
| } | ||
|
|
||
| @Test | ||
| public void testRootAllocator_listeners() throws Exception { | ||
| TestAllocationListener l1 = new TestAllocationListener(); | ||
| assertEquals(0, l1.getNumCalls()); | ||
| assertEquals(0, l1.getTotalMem()); | ||
| TestAllocationListener l2 = new TestAllocationListener(); | ||
| assertEquals(0, l2.getNumCalls()); | ||
| assertEquals(0, l2.getTotalMem()); | ||
| // root and first-level child share the first listener | ||
| // second-level and third-level child share the second listener | ||
| try (final RootAllocator rootAllocator = new RootAllocator(l1, MAX_ALLOCATION)) { | ||
| try (final BufferAllocator c1 = rootAllocator.newChildAllocator("c1", 0, MAX_ALLOCATION)) { | ||
| final ArrowBuf buf1 = c1.buffer(16); | ||
| assertNotNull("allocation failed", buf1); | ||
| assertEquals(1, l1.getNumCalls()); | ||
| assertEquals(16, l1.getTotalMem()); | ||
| buf1.release(); | ||
| try (final BufferAllocator c2 = c1.newChildAllocator("c2", l2, 0, MAX_ALLOCATION)) { | ||
| final ArrowBuf buf2 = c2.buffer(32); | ||
| assertNotNull("allocation failed", buf2); | ||
| assertEquals(1, l1.getNumCalls()); | ||
| assertEquals(16, l1.getTotalMem()); | ||
| assertEquals(1, l2.getNumCalls()); | ||
| assertEquals(32, l2.getTotalMem()); | ||
| buf2.release(); | ||
| try (final BufferAllocator c3 = c2.newChildAllocator("c3", 0, MAX_ALLOCATION)) { | ||
| final ArrowBuf buf3 = c3.buffer(64); | ||
| assertNotNull("allocation failed", buf3); | ||
| assertEquals(1, l1.getNumCalls()); | ||
| assertEquals(16, l1.getTotalMem()); | ||
| assertEquals(2, l2.getNumCalls()); | ||
| assertEquals(32 + 64, l2.getTotalMem()); | ||
| buf3.release(); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| @Test | ||
| public void testRootAllocator_listenerAllocationFail() throws Exception { | ||
| TestAllocationListener l1 = new TestAllocationListener(); | ||
| assertEquals(0, l1.getNumCalls()); | ||
| assertEquals(0, l1.getTotalMem()); | ||
| // Test attempts to allocate too much from a child whose limit is set to half of the max allocation | ||
| // The listener's callback triggers, expanding the child allocator's limit, so then the allocation succeeds | ||
| try (final RootAllocator rootAllocator = new RootAllocator(MAX_ALLOCATION)) { | ||
| try (final BufferAllocator c1 = rootAllocator.newChildAllocator("c1", l1,0, MAX_ALLOCATION / 2)) { | ||
| try { | ||
| c1.buffer(MAX_ALLOCATION); | ||
| fail("allocated memory beyond max allowed"); | ||
| } catch (OutOfMemoryException e) { | ||
| // expected | ||
| } | ||
| assertEquals(0, l1.getNumCalls()); | ||
| assertEquals(0, l1.getTotalMem()); | ||
|
|
||
| l1.setExpandOnFail(c1, MAX_ALLOCATION); | ||
| ArrowBuf arrowBuf = c1.buffer(MAX_ALLOCATION); | ||
| assertNotNull("allocation failed", arrowBuf); | ||
| assertEquals(1, l1.getNumCalls()); | ||
| assertEquals(MAX_ALLOCATION, l1.getTotalMem()); | ||
| arrowBuf.release(); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| private static void allocateAndFree(final BufferAllocator allocator) { | ||
| final ArrowBuf arrowBuf = allocator.buffer(512); | ||
| assertNotNull("allocation failed", arrowBuf); | ||
| arrowBuf.release(); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we update the protected constructor/consolidate? Would rather not expose more constructors.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure how to update/consolidate, short of removing the previous two protected constructors -- which breaks the public interface, and thus probably not the best idea. I cannot use the existing constructors since I need to support listener instances that are not common to the entire hierarchy.
Suggestions are welcome.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it is okay changing the constructor signature of BaseAllocator and consolidate overlap