You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "zhztheplayer (via GitHub)" <gi...@apache.org> on 2023/02/17 03:40:25 UTC
[GitHub] [arrow] zhztheplayer opened a new pull request, #34231: GH-34230: [Java] Call allocation listener on BaseAllocator#wrapForeignAllocation
zhztheplayer opened a new pull request, #34231:
URL: https://github.com/apache/arrow/pull/34231
#34230
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lidavidm commented on a diff in pull request #34231: GH-34230: [Java] Call allocation listener on BaseAllocator#wrapForeignAllocation
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #34231:
URL: https://github.com/apache/arrow/pull/34231#discussion_r1111415314
##########
java/memory/memory-core/src/main/java/org/apache/arrow/memory/BaseAllocator.java:
##########
@@ -233,6 +233,41 @@ private void childClosed(final BaseAllocator childAllocator) {
listener.onChildRemoved(this, childAllocator);
}
+ @Override
+ public ArrowBuf wrapForeignAllocation(ForeignAllocation allocation) {
+ assertOpen();
+ final long size = allocation.getSize();
+ listener.onPreAllocation(size);
+ AllocationOutcome outcome = this.allocateBytes(size);
Review Comment:
As long as it's consistent. I think the behavior here is OK, and consistent with the "allocator" actually being a "ledger".
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lidavidm commented on a diff in pull request #34231: GH-34230: [Java] Call allocation listener on BaseAllocator#wrapForeignAllocation
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #34231:
URL: https://github.com/apache/arrow/pull/34231#discussion_r1109781042
##########
java/memory/memory-netty/src/test/java/org/apache/arrow/memory/CountingAllocationListener.java:
##########
@@ -0,0 +1,111 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.arrow.memory;
+
+// Allocation listener
Review Comment:
nit, but please stick to JavaDoc syntax for this?
##########
java/memory/memory-core/src/main/java/org/apache/arrow/memory/BufferAllocator.java:
##########
@@ -248,22 +248,5 @@ default RoundingPolicy getRoundingPolicy() {
*
* @param allocation The underlying allocation.
*/
- default ArrowBuf wrapForeignAllocation(ForeignAllocation allocation) {
- try {
- forceAllocate(allocation.getSize());
- final AllocationManager manager = new ForeignAllocationManager(this, allocation);
- final BufferLedger ledger = manager.associate(this);
- final ArrowBuf buf =
- new ArrowBuf(ledger, /*bufferManager=*/null, allocation.getSize(), allocation.memoryAddress());
- buf.writerIndex(allocation.getSize());
- return buf;
- } catch (Throwable t) {
- try {
- allocation.release0();
- } catch (Throwable e) {
- t.addSuppressed(e);
- }
- throw t;
- }
- }
+ ArrowBuf wrapForeignAllocation(ForeignAllocation allocation);
Review Comment:
This is a breaking change. Can we at least have a default implementation that throws UnsupportedOperationException?
##########
java/memory/memory-netty/src/test/java/org/apache/arrow/memory/TestForeignAllocation.java:
##########
@@ -54,6 +54,55 @@ public void wrapForeignAllocation() {
assertEquals(0, allocator.getAllocatedMemory());
}
+ @Test
+ public void wrapForeignAllocationWithAllocationListener() {
+ final long bufferSize = 16;
+
+ final CountingAllocationListener listener = new CountingAllocationListener();
+ try (BufferAllocator listenedAllocator =
+ allocator.newChildAllocator("child", listener, 0L, allocator.getLimit())) {
+ UnsafeForeignAllocation allocation = new UnsafeForeignAllocation(bufferSize);
+ try {
+ assertEquals(0, listenedAllocator.getAllocatedMemory());
+ ArrowBuf buf = listenedAllocator.wrapForeignAllocation(allocation);
+ assertEquals(bufferSize, buf.capacity());
+ assertEquals(16, listener.getCurrentMem());
+ buf.close();
+ assertEquals(0, listener.getCurrentMem());
+ assertTrue(allocation.released);
+ } finally {
+ allocation.release0();
+ }
+ assertEquals(0, listenedAllocator.getAllocatedMemory());
+ }
+ assertEquals(1, listener.getNumPreCalls());
+ assertEquals(1, listener.getNumCalls());
+ assertEquals(1, listener.getNumReleaseCalls());
+ assertEquals(16, listener.getTotalMem());
+ }
+
+ @Test(expected = OutOfMemoryException.class)
+ public void wrapForeignAllocationFailedWithAllocationListener() {
Review Comment:
Can we also test the case where the listener frees some memory and the allocation succeeds?
##########
java/memory/memory-core/src/main/java/org/apache/arrow/memory/BaseAllocator.java:
##########
@@ -233,6 +233,41 @@ private void childClosed(final BaseAllocator childAllocator) {
listener.onChildRemoved(this, childAllocator);
}
+ @Override
+ public ArrowBuf wrapForeignAllocation(ForeignAllocation allocation) {
+ assertOpen();
+ final long size = allocation.getSize();
+ listener.onPreAllocation(size);
+ AllocationOutcome outcome = this.allocateBytes(size);
Review Comment:
Hmm, if we don't force allocation the behavior is different - the memory is already allocated, we're just trying to account for it. This will 'eagerly' release resources (via the listener) to make room while the existing behavior is to do it 'lazily' (the next allocation will trigger things if needed). Is that what you want?
##########
java/memory/memory-core/src/main/java/org/apache/arrow/memory/BaseAllocator.java:
##########
@@ -233,6 +233,41 @@ private void childClosed(final BaseAllocator childAllocator) {
listener.onChildRemoved(this, childAllocator);
}
+ @Override
+ public ArrowBuf wrapForeignAllocation(ForeignAllocation allocation) {
+ assertOpen();
+ final long size = allocation.getSize();
+ listener.onPreAllocation(size);
+ AllocationOutcome outcome = this.allocateBytes(size);
+ if (!outcome.isOk()) {
+ if (listener.onFailedAllocation(size, outcome)) {
+ // Second try, in case the listener can do something about it
+ outcome = this.allocateBytes(size);
+ }
+ if (!outcome.isOk()) {
+ throw new OutOfMemoryException(createErrorMsg(this, size,
+ size), outcome.getDetails());
+ }
+ }
+ try {
+ final AllocationManager manager = new ForeignAllocationManager(this, allocation);
+ final BufferLedger ledger = manager.associate(this);
+ final ArrowBuf buf =
+ new ArrowBuf(ledger, /*bufferManager=*/null, size, allocation.memoryAddress());
+ buf.writerIndex(size);
+ listener.onAllocation(size);
+ return buf;
+ } catch (Throwable t) {
+ try {
+ releaseBytes(size);
+ allocation.release0();
Review Comment:
can this be in a separate try-catch?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] zhztheplayer commented on a diff in pull request #34231: GH-34230: [Java] Call allocation listener on BaseAllocator#wrapForeignAllocation
Posted by "zhztheplayer (via GitHub)" <gi...@apache.org>.
zhztheplayer commented on code in PR #34231:
URL: https://github.com/apache/arrow/pull/34231#discussion_r1111398760
##########
java/memory/memory-netty/src/test/java/org/apache/arrow/memory/TestForeignAllocation.java:
##########
@@ -54,6 +54,55 @@ public void wrapForeignAllocation() {
assertEquals(0, allocator.getAllocatedMemory());
}
+ @Test
+ public void wrapForeignAllocationWithAllocationListener() {
+ final long bufferSize = 16;
+
+ final CountingAllocationListener listener = new CountingAllocationListener();
+ try (BufferAllocator listenedAllocator =
+ allocator.newChildAllocator("child", listener, 0L, allocator.getLimit())) {
+ UnsafeForeignAllocation allocation = new UnsafeForeignAllocation(bufferSize);
+ try {
+ assertEquals(0, listenedAllocator.getAllocatedMemory());
+ ArrowBuf buf = listenedAllocator.wrapForeignAllocation(allocation);
+ assertEquals(bufferSize, buf.capacity());
+ assertEquals(16, listener.getCurrentMem());
+ buf.close();
+ assertEquals(0, listener.getCurrentMem());
+ assertTrue(allocation.released);
+ } finally {
+ allocation.release0();
+ }
+ assertEquals(0, listenedAllocator.getAllocatedMemory());
+ }
+ assertEquals(1, listener.getNumPreCalls());
+ assertEquals(1, listener.getNumCalls());
+ assertEquals(1, listener.getNumReleaseCalls());
+ assertEquals(16, listener.getTotalMem());
+ }
+
+ @Test(expected = OutOfMemoryException.class)
+ public void wrapForeignAllocationFailedWithAllocationListener() {
Review Comment:
Sure, I'll add a case
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lidavidm merged pull request #34231: GH-34230: [Java] Call allocation listener on BaseAllocator#wrapForeignAllocation
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm merged PR #34231:
URL: https://github.com/apache/arrow/pull/34231
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #34231: GH-34230: [Java] Call allocation listener on BaseAllocator#wrapForeignAllocation
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #34231:
URL: https://github.com/apache/arrow/pull/34231#issuecomment-1434052979
* Closes: #34230
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] zhztheplayer commented on a diff in pull request #34231: GH-34230: [Java] Call allocation listener on BaseAllocator#wrapForeignAllocation
Posted by "zhztheplayer (via GitHub)" <gi...@apache.org>.
zhztheplayer commented on code in PR #34231:
URL: https://github.com/apache/arrow/pull/34231#discussion_r1111563849
##########
java/memory/memory-core/src/main/java/org/apache/arrow/memory/BaseAllocator.java:
##########
@@ -233,6 +233,41 @@ private void childClosed(final BaseAllocator childAllocator) {
listener.onChildRemoved(this, childAllocator);
}
+ @Override
+ public ArrowBuf wrapForeignAllocation(ForeignAllocation allocation) {
+ assertOpen();
+ final long size = allocation.getSize();
+ listener.onPreAllocation(size);
+ AllocationOutcome outcome = this.allocateBytes(size);
+ if (!outcome.isOk()) {
+ if (listener.onFailedAllocation(size, outcome)) {
+ // Second try, in case the listener can do something about it
+ outcome = this.allocateBytes(size);
+ }
+ if (!outcome.isOk()) {
+ throw new OutOfMemoryException(createErrorMsg(this, size,
+ size), outcome.getDetails());
+ }
+ }
+ try {
+ final AllocationManager manager = new ForeignAllocationManager(this, allocation);
+ final BufferLedger ledger = manager.associate(this);
+ final ArrowBuf buf =
+ new ArrowBuf(ledger, /*bufferManager=*/null, size, allocation.memoryAddress());
+ buf.writerIndex(size);
+ listener.onAllocation(size);
+ return buf;
+ } catch (Throwable t) {
+ try {
+ releaseBytes(size);
+ allocation.release0();
Review Comment:
Thanks. Updated.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] ursabot commented on pull request #34231: GH-34230: [Java] Call allocation listener on BaseAllocator#wrapForeignAllocation
Posted by "ursabot (via GitHub)" <gi...@apache.org>.
ursabot commented on PR #34231:
URL: https://github.com/apache/arrow/pull/34231#issuecomment-1438768403
Benchmark runs are scheduled for baseline = 1d74483fa0659aebc0cb1dfb771ba38800166bf2 and contender = 6850923cc56c57dac28c85088d9c49789f9ecfdc. 6850923cc56c57dac28c85088d9c49789f9ecfdc is a master commit associated with this PR. Results will be available as each benchmark for each run completes.
Conbench compare runs links:
[Finished :arrow_down:0.0% :arrow_up:0.0%] [ec2-t3-xlarge-us-east-2](https://conbench.ursa.dev/compare/runs/65cf4a9bb19643bda7ea3a469783b8d5...1425e4a7d62e46daa44c664a574c5217/)
[Failed :arrow_down:0.31% :arrow_up:0.06%] [test-mac-arm](https://conbench.ursa.dev/compare/runs/d459224a0c2945f58620b4a0a105a13a...b5db3ecc8ba942349fffaaae59e02a8c/)
[Finished :arrow_down:0.0% :arrow_up:0.0%] [ursa-i9-9960x](https://conbench.ursa.dev/compare/runs/db3a4a73e0a24c21b0e679b880a641df...e3e718594eb64d1cb7856deab8f471c3/)
[Finished :arrow_down:0.19% :arrow_up:0.03%] [ursa-thinkcentre-m75q](https://conbench.ursa.dev/compare/runs/041dd121ac9a46fea63fbfd2d4ce6289...aadc53b690bb48daade901b4adb3550b/)
Buildkite builds:
[Finished] [`6850923c` ec2-t3-xlarge-us-east-2](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/2405)
[Failed] [`6850923c` test-mac-arm](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/2435)
[Finished] [`6850923c` ursa-i9-9960x](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/2403)
[Finished] [`6850923c` ursa-thinkcentre-m75q](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/2427)
[Finished] [`1d74483f` ec2-t3-xlarge-us-east-2](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ec2-t3-xlarge-us-east-2/builds/2404)
[Finished] [`1d74483f` test-mac-arm](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-test-mac-arm/builds/2434)
[Finished] [`1d74483f` ursa-i9-9960x](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-i9-9960x/builds/2402)
[Finished] [`1d74483f` ursa-thinkcentre-m75q](https://buildkite.com/apache-arrow/arrow-bci-benchmark-on-ursa-thinkcentre-m75q/builds/2426)
Supported benchmarks:
ec2-t3-xlarge-us-east-2: Supported benchmark langs: Python, R. Runs only benchmarks with cloud = True
test-mac-arm: Supported benchmark langs: C++, Python, R
ursa-i9-9960x: Supported benchmark langs: Python, R, JavaScript
ursa-thinkcentre-m75q: Supported benchmark langs: C++, Java
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] zhztheplayer commented on a diff in pull request #34231: GH-34230: [Java] Call allocation listener on BaseAllocator#wrapForeignAllocation
Posted by "zhztheplayer (via GitHub)" <gi...@apache.org>.
zhztheplayer commented on code in PR #34231:
URL: https://github.com/apache/arrow/pull/34231#discussion_r1111404207
##########
java/memory/memory-core/src/main/java/org/apache/arrow/memory/BaseAllocator.java:
##########
@@ -233,6 +233,41 @@ private void childClosed(final BaseAllocator childAllocator) {
listener.onChildRemoved(this, childAllocator);
}
+ @Override
+ public ArrowBuf wrapForeignAllocation(ForeignAllocation allocation) {
+ assertOpen();
+ final long size = allocation.getSize();
+ listener.onPreAllocation(size);
+ AllocationOutcome outcome = this.allocateBytes(size);
+ if (!outcome.isOk()) {
+ if (listener.onFailedAllocation(size, outcome)) {
+ // Second try, in case the listener can do something about it
+ outcome = this.allocateBytes(size);
+ }
+ if (!outcome.isOk()) {
+ throw new OutOfMemoryException(createErrorMsg(this, size,
+ size), outcome.getDetails());
+ }
+ }
+ try {
+ final AllocationManager manager = new ForeignAllocationManager(this, allocation);
+ final BufferLedger ledger = manager.associate(this);
+ final ArrowBuf buf =
+ new ArrowBuf(ledger, /*bufferManager=*/null, size, allocation.memoryAddress());
+ buf.writerIndex(size);
+ listener.onAllocation(size);
+ return buf;
+ } catch (Throwable t) {
+ try {
+ releaseBytes(size);
+ allocation.release0();
Review Comment:
Thanks but would you give some hints on this? Or would you suggest adding a local variable `Throwable xx` to save the error for further handling?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] zhztheplayer commented on a diff in pull request #34231: GH-34230: [Java] Call allocation listener on BaseAllocator#wrapForeignAllocation
Posted by "zhztheplayer (via GitHub)" <gi...@apache.org>.
zhztheplayer commented on code in PR #34231:
URL: https://github.com/apache/arrow/pull/34231#discussion_r1111383894
##########
java/memory/memory-core/src/main/java/org/apache/arrow/memory/BaseAllocator.java:
##########
@@ -233,6 +233,41 @@ private void childClosed(final BaseAllocator childAllocator) {
listener.onChildRemoved(this, childAllocator);
}
+ @Override
+ public ArrowBuf wrapForeignAllocation(ForeignAllocation allocation) {
+ assertOpen();
+ final long size = allocation.getSize();
+ listener.onPreAllocation(size);
+ AllocationOutcome outcome = this.allocateBytes(size);
Review Comment:
Thanks for noticing this. I think it's not a very big issue as long as we treat "accountation of foreign allocation" as another type of "allocation" too. Since sometimes it can be tricky to track on actual foreign allocation at real-time from Java side.
Also another solution would be just excluding foreign allocation from `listener.release()` too. But in that way one will lose tracking if he didn't set up a independent path to report the foreign allocations.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] zhztheplayer commented on pull request #34231: GH-34230: [Java] Call allocation listener on BaseAllocator#wrapForeignAllocation
Posted by "zhztheplayer (via GitHub)" <gi...@apache.org>.
zhztheplayer commented on PR #34231:
URL: https://github.com/apache/arrow/pull/34231#issuecomment-1436215678
@lidavidm Would you like to take another look? Thanks.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] github-actions[bot] commented on pull request #34231: GH-34230: [Java] Call allocation listener on BaseAllocator#wrapForeignAllocation
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #34231:
URL: https://github.com/apache/arrow/pull/34231#issuecomment-1434053001
:warning: GitHub issue #34230 **has been automatically assigned in GitHub** to PR creator.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lidavidm commented on a diff in pull request #34231: GH-34230: [Java] Call allocation listener on BaseAllocator#wrapForeignAllocation
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #34231:
URL: https://github.com/apache/arrow/pull/34231#discussion_r1111970829
##########
java/memory/memory-netty/src/test/java/org/apache/arrow/memory/TestForeignAllocation.java:
##########
@@ -54,6 +57,80 @@ public void wrapForeignAllocation() {
assertEquals(0, allocator.getAllocatedMemory());
}
+ @Test
+ public void wrapForeignAllocationWithAllocationListener() {
+ final long bufferSize = 16;
+
+ final CountingAllocationListener listener = new CountingAllocationListener();
+ try (BufferAllocator listenedAllocator =
+ allocator.newChildAllocator("child", listener, 0L, allocator.getLimit())) {
+ UnsafeForeignAllocation allocation = new UnsafeForeignAllocation(bufferSize);
+ try {
+ assertEquals(0, listenedAllocator.getAllocatedMemory());
+ ArrowBuf buf = listenedAllocator.wrapForeignAllocation(allocation);
+ assertEquals(bufferSize, buf.capacity());
+ assertEquals(16, listener.getCurrentMem());
+ buf.close();
+ assertEquals(0, listener.getCurrentMem());
+ assertTrue(allocation.released);
+ } finally {
+ allocation.release0();
+ }
+ assertEquals(0, listenedAllocator.getAllocatedMemory());
+ }
+ assertEquals(1, listener.getNumPreCalls());
+ assertEquals(1, listener.getNumCalls());
+ assertEquals(1, listener.getNumReleaseCalls());
+ assertEquals(16, listener.getTotalMem());
+ }
+
+ @Test(expected = OutOfMemoryException.class)
+ public void wrapForeignAllocationFailedWithAllocationListener() {
+ final long bufferSize = 16;
+ final long limit = bufferSize - 1;
+
+ final CountingAllocationListener listener = new CountingAllocationListener();
+ try (BufferAllocator listenedAllocator =
+ allocator.newChildAllocator("child", listener, 0L, limit)) {
+ UnsafeForeignAllocation allocation = new UnsafeForeignAllocation(bufferSize);
+ try {
+ assertEquals(0, listenedAllocator.getAllocatedMemory());
+ ArrowBuf buf = listenedAllocator.wrapForeignAllocation(allocation);
+ assertEquals(bufferSize, buf.capacity());
+ buf.close();
+ assertTrue(allocation.released);
+ } finally {
+ allocation.release0();
+ }
+ }
+ }
+
+ @Test
+ public void wrapForeignAllocationWithAllocationListenerReclaimingSpace() {
+ final long bufferSize = 16;
+ final long limit = 2 * bufferSize - 1;
+
+ final List<ArrowBuf> bufferedToBeFreed = new ArrayList<>();
Review Comment:
nit: typo
```suggestion
final List<ArrowBuf> buffersToBeFreed = new ArrayList<>();
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [arrow] lidavidm commented on a diff in pull request #34231: GH-34230: [Java] Call allocation listener on BaseAllocator#wrapForeignAllocation
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #34231:
URL: https://github.com/apache/arrow/pull/34231#discussion_r1111414917
##########
java/memory/memory-core/src/main/java/org/apache/arrow/memory/BaseAllocator.java:
##########
@@ -233,6 +233,41 @@ private void childClosed(final BaseAllocator childAllocator) {
listener.onChildRemoved(this, childAllocator);
}
+ @Override
+ public ArrowBuf wrapForeignAllocation(ForeignAllocation allocation) {
+ assertOpen();
+ final long size = allocation.getSize();
+ listener.onPreAllocation(size);
+ AllocationOutcome outcome = this.allocateBytes(size);
+ if (!outcome.isOk()) {
+ if (listener.onFailedAllocation(size, outcome)) {
+ // Second try, in case the listener can do something about it
+ outcome = this.allocateBytes(size);
+ }
+ if (!outcome.isOk()) {
+ throw new OutOfMemoryException(createErrorMsg(this, size,
+ size), outcome.getDetails());
+ }
+ }
+ try {
+ final AllocationManager manager = new ForeignAllocationManager(this, allocation);
+ final BufferLedger ledger = manager.associate(this);
+ final ArrowBuf buf =
+ new ArrowBuf(ledger, /*bufferManager=*/null, size, allocation.memoryAddress());
+ buf.writerIndex(size);
+ listener.onAllocation(size);
+ return buf;
+ } catch (Throwable t) {
+ try {
+ releaseBytes(size);
+ allocation.release0();
Review Comment:
What I mean is that `release0` should be called even if `releaseBytes` throws.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org