You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "davisusanibar (via GitHub)" <gi...@apache.org> on 2023/08/15 16:03:54 UTC
[GitHub] [arrow] davisusanibar opened a new pull request, #37178: GH-37177: [Java][Doc] Enable singleton allocator
davisusanibar opened a new pull request, #37178:
URL: https://github.com/apache/arrow/pull/37178
### Rationale for this change
Upstream SingletonAllocator
To close https://github.com/apache/arrow/issues/37177
### What changes are included in this PR?
Enable SingletonAllocator utility
### Are these changes tested?
Yes
### Are there any user-facing changes?
No
--
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] adamkennedy commented on a diff in pull request #37178: GH-37177: [Java][Docs] Enable singleton allocator
Posted by "adamkennedy (via GitHub)" <gi...@apache.org>.
adamkennedy commented on code in PR #37178:
URL: https://github.com/apache/arrow/pull/37178#discussion_r1296342646
##########
docs/source/java/memory.rst:
##########
@@ -452,3 +452,5 @@ subset of that memory (e.g. through slicing) might be different. Also
note that ArrowBuf 2 and ArrowBuf 4, 5 and 6 are also sharing the same
underlying memory. Also note that ArrowBuf 4, 5 and 6 all share the same
reference count and fate.
+
+.. _`Singleton Allocator`: https://arrow.apache.org/docs/java/reference/org/apache/arrow/memory/util/SingletonAllocator.html
Review Comment:
This name and link are wrong now right?
--
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] adamkennedy commented on a diff in pull request #37178: GH-37177: [Java][Docs] Enable singleton allocator
Posted by "adamkennedy (via GitHub)" <gi...@apache.org>.
adamkennedy commented on code in PR #37178:
URL: https://github.com/apache/arrow/pull/37178#discussion_r1296346656
##########
java/memory/memory-core/src/main/java/org/apache/arrow/memory/util/GlobalAllocator.java:
##########
@@ -0,0 +1,99 @@
+/*
+ * 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.util;
+
+import java.util.concurrent.atomic.AtomicInteger;
+import java.util.stream.Collectors;
+
+import org.apache.arrow.memory.BufferAllocator;
+import org.apache.arrow.memory.RootAllocator;
+import org.apache.arrow.util.VisibleForTesting;
+
+/**
+ * Default implementation for memory allocation in case you are using a global
+ * allocator, otherwise, you should pass root allocator explicitly on demand.
+ */
+public final class GlobalAllocator {
+ private GlobalAllocator() {}
+
+ private static final RootAllocator GLOBAL_ROOT_ALLOCATOR = new RootAllocator();
+ private static final AtomicInteger CHILD_NUMBER = new AtomicInteger(0);
+
+ /**
+ * Get default new child allocator.
+ *
+ * @return a new child {@link BufferAllocator}.
+ */
+ public static BufferAllocator getChildAllocator() {
+ return getChildAllocator(0, Long.MAX_VALUE);
+ }
+
+ /**
+ * Get custom new child allocator with initial reservation and maximum allocation.
+ *
+ * @return a new child {@link BufferAllocator}.
+ */
+ public static BufferAllocator getChildAllocator(final long initReservation, final long maxAllocation) {
+ return GLOBAL_ROOT_ALLOCATOR.newChildAllocator(nextChildName(), initReservation, maxAllocation);
+ }
+
+ private static String nextChildName() {
+ return "ChildAllocator-" + CHILD_NUMBER.incrementAndGet();
Review Comment:
Doesn't have a lot of meaning here because "ChildAllocator" has no obvious relation to the GlobalAllocator.
Perhaps explicitly call out the source of the child in the name? `"GlobalAllocator-"` or `"GlobalAllocator-Child#"` or something?
--
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 #37178: GH-37177: [Java][Docs] Enable singleton allocator
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #37178:
URL: https://github.com/apache/arrow/pull/37178#discussion_r1296201718
##########
java/memory/memory-core/src/main/java/org/apache/arrow/memory/util/SingletonAllocator.java:
##########
@@ -0,0 +1,103 @@
+/*
+ * 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.util;
+
+import java.util.concurrent.atomic.AtomicInteger;
+import java.util.stream.Collectors;
+
+import org.apache.arrow.memory.BufferAllocator;
+import org.apache.arrow.memory.RootAllocator;
+import org.apache.arrow.util.VisibleForTesting;
+
+/**
+ * Factory class for managing allocators in simple applications that make
+ * use of an application-wide singleton root allocator with no listeners,
+ * automatic allocator defaults, and no total allocator limit.
+ * <p>
+ * Child allocators are created with a default name with the format
+ * <code>SingletonAllocator#(AtomicCounter)</code>.
+ */
+public final class SingletonAllocator {
Review Comment:
We need to ban this from usage in Arrow itself. I'm also not convinced this is worth shipping as part of Arrow, vs improving documentation on how allocators are supposed to be used.
##########
java/memory/memory-core/src/test/java/org/apache/arrow/memory/util/SingletonAllocatorTest.java:
##########
@@ -0,0 +1,82 @@
+/*
+ * 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.util;
+
+import static org.apache.arrow.memory.util.SingletonAllocator.checkGlobalCleanUpResources;
+import static org.apache.arrow.memory.util.SingletonAllocator.getAllocatedMemory;
+import static org.apache.arrow.memory.util.SingletonAllocator.getAllocator;
+import static org.apache.arrow.memory.util.SingletonAllocator.hasActiveAllocators;
+import static org.apache.arrow.memory.util.SingletonAllocator.verify;
Review Comment:
Don't static import methods.
##########
java/memory/memory-core/src/main/java/org/apache/arrow/memory/util/SingletonAllocator.java:
##########
@@ -0,0 +1,103 @@
+/*
+ * 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.util;
+
+import java.util.concurrent.atomic.AtomicInteger;
+import java.util.stream.Collectors;
+
+import org.apache.arrow.memory.BufferAllocator;
+import org.apache.arrow.memory.RootAllocator;
+import org.apache.arrow.util.VisibleForTesting;
+
+/**
+ * Factory class for managing allocators in simple applications that make
+ * use of an application-wide singleton root allocator with no listeners,
+ * automatic allocator defaults, and no total allocator limit.
Review Comment:
I think a better way of describing it is this is a pattern for applications that wish to use a global singleton allocator instead of explicitly passing around the root allocator. It should be preferred to creating a new root allocator on the fly.
##########
java/memory/memory-core/src/main/java/org/apache/arrow/memory/util/SingletonAllocator.java:
##########
@@ -0,0 +1,103 @@
+/*
+ * 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.util;
+
+import java.util.concurrent.atomic.AtomicInteger;
+import java.util.stream.Collectors;
+
+import org.apache.arrow.memory.BufferAllocator;
+import org.apache.arrow.memory.RootAllocator;
+import org.apache.arrow.util.VisibleForTesting;
+
+/**
+ * Factory class for managing allocators in simple applications that make
+ * use of an application-wide singleton root allocator with no listeners,
+ * automatic allocator defaults, and no total allocator limit.
+ * <p>
+ * Child allocators are created with a default name with the format
+ * <code>SingletonAllocator#(AtomicCounter)</code>.
+ */
+public final class SingletonAllocator {
+ private SingletonAllocator() {}
+
+ private static final RootAllocator ROOT = new RootAllocator();
+ private static final AtomicInteger COUNTER = new AtomicInteger(0);
+
+ /**
+ * Get a new child allocator with a default name, no initial reservation and no maximum allocation.
+ *
+ * @return a new child {@link BufferAllocator}.
+ */
+ public static BufferAllocator getAllocator() {
+ return getAllocator(0, Long.MAX_VALUE);
+ }
+
+ /**
+ * Get a new child allocator with a default name and a custom initial reservation and maximum allocation.
+ *
+ * @return a new child {@link BufferAllocator}.
+ */
+ public static BufferAllocator getAllocator(final long initReservation, final long maxAllocation) {
+ return ROOT.newChildAllocator(nextChildName(), initReservation, maxAllocation);
+ }
+
+ private static String nextChildName() {
+ return "SingletonAllocator#" + COUNTER.incrementAndGet();
+ }
+
+ /**
+ * Get the total allocated memory across all child allocators.
+ *
+ * @return the number of allocated bytes.
+ */
+ public static long getAllocatedMemory() {
+ return ROOT.getAllocatedMemory();
+ }
+
+ /**
+ * Check if there are any outstanding allocators that have not been closed.
+ *
+ * @return <code>true</code> if child allocators exist or <code>false</code> if not.
+ */
+ public static boolean hasActiveAllocators() {
+ return !ROOT.getChildAllocators().isEmpty();
+ }
+
+ @VisibleForTesting
+ static void checkGlobalCleanUpResources() {
Review Comment:
Probably this should be public? An application should check that all memory is freed at shutdown
##########
docs/source/java/memory.rst:
##########
@@ -133,7 +133,7 @@ Development Guidelines
Applications should generally:
* Use the BufferAllocator interface in APIs instead of RootAllocator.
-* Create one RootAllocator at the start of the program.
+* Create one RootAllocator at the start of the program (in case `Singleton Allocator`_ is needed, consider using it).
Review Comment:
I would generally recommend against using it...
--
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] adamkennedy commented on a diff in pull request #37178: GH-37177: [Java][Docs] Enable singleton allocator
Posted by "adamkennedy (via GitHub)" <gi...@apache.org>.
adamkennedy commented on code in PR #37178:
URL: https://github.com/apache/arrow/pull/37178#discussion_r1296339566
##########
java/memory/memory-core/src/main/java/org/apache/arrow/memory/util/GlobalAllocator.java:
##########
@@ -0,0 +1,99 @@
+/*
+ * 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.util;
+
+import java.util.concurrent.atomic.AtomicInteger;
+import java.util.stream.Collectors;
+
+import org.apache.arrow.memory.BufferAllocator;
+import org.apache.arrow.memory.RootAllocator;
+import org.apache.arrow.util.VisibleForTesting;
+
+/**
+ * Default implementation for memory allocation in case you are using a global
+ * allocator, otherwise, you should pass root allocator explicitly on demand.
+ */
Review Comment:
Do we want to do a longer description of the value and risks of using the GlobalAllocator?
--
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] davisusanibar closed pull request #37178: GH-37177: [Java][Docs] Enable singleton allocator
Posted by "davisusanibar (via GitHub)" <gi...@apache.org>.
davisusanibar closed pull request #37178: GH-37177: [Java][Docs] Enable singleton allocator
URL: https://github.com/apache/arrow/pull/37178
--
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] davisusanibar commented on a diff in pull request #37178: GH-37177: [Java][Docs] Enable singleton allocator
Posted by "davisusanibar (via GitHub)" <gi...@apache.org>.
davisusanibar commented on code in PR #37178:
URL: https://github.com/apache/arrow/pull/37178#discussion_r1296327761
##########
docs/source/java/memory.rst:
##########
@@ -133,7 +133,7 @@ Development Guidelines
Applications should generally:
* Use the BufferAllocator interface in APIs instead of RootAllocator.
-* Create one RootAllocator at the start of the program.
+* Create one RootAllocator at the start of the program (in case `Singleton Allocator`_ is needed, consider using it).
Review Comment:
Deleted
##########
java/memory/memory-core/src/test/java/org/apache/arrow/memory/util/SingletonAllocatorTest.java:
##########
@@ -0,0 +1,82 @@
+/*
+ * 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.util;
+
+import static org.apache.arrow.memory.util.SingletonAllocator.checkGlobalCleanUpResources;
+import static org.apache.arrow.memory.util.SingletonAllocator.getAllocatedMemory;
+import static org.apache.arrow.memory.util.SingletonAllocator.getAllocator;
+import static org.apache.arrow.memory.util.SingletonAllocator.hasActiveAllocators;
+import static org.apache.arrow.memory.util.SingletonAllocator.verify;
Review Comment:
Deleted
--
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] adamkennedy commented on a diff in pull request #37178: GH-37177: [Java][Docs] Enable singleton allocator
Posted by "adamkennedy (via GitHub)" <gi...@apache.org>.
adamkennedy commented on code in PR #37178:
URL: https://github.com/apache/arrow/pull/37178#discussion_r1296337266
##########
docs/source/java/memory.rst:
##########
@@ -133,7 +133,7 @@ Development Guidelines
Applications should generally:
* Use the BufferAllocator interface in APIs instead of RootAllocator.
-* Create one RootAllocator at the start of the program.
+* Create one RootAllocator at the start of the program and explicitly passing it when it is needed.
Review Comment:
Grammar "and explicitly pass it when needed"
--
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] davisusanibar commented on a diff in pull request #37178: GH-37177: [Java][Docs] Enable singleton allocator
Posted by "davisusanibar (via GitHub)" <gi...@apache.org>.
davisusanibar commented on code in PR #37178:
URL: https://github.com/apache/arrow/pull/37178#discussion_r1296248401
##########
java/memory/memory-core/src/main/java/org/apache/arrow/memory/util/SingletonAllocator.java:
##########
@@ -0,0 +1,103 @@
+/*
+ * 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.util;
+
+import java.util.concurrent.atomic.AtomicInteger;
+import java.util.stream.Collectors;
+
+import org.apache.arrow.memory.BufferAllocator;
+import org.apache.arrow.memory.RootAllocator;
+import org.apache.arrow.util.VisibleForTesting;
+
+/**
+ * Factory class for managing allocators in simple applications that make
+ * use of an application-wide singleton root allocator with no listeners,
+ * automatic allocator defaults, and no total allocator limit.
+ * <p>
+ * Child allocators are created with a default name with the format
+ * <code>SingletonAllocator#(AtomicCounter)</code>.
+ */
+public final class SingletonAllocator {
+ private SingletonAllocator() {}
+
+ private static final RootAllocator ROOT = new RootAllocator();
+ private static final AtomicInteger COUNTER = new AtomicInteger(0);
+
+ /**
+ * Get a new child allocator with a default name, no initial reservation and no maximum allocation.
+ *
+ * @return a new child {@link BufferAllocator}.
+ */
+ public static BufferAllocator getAllocator() {
+ return getAllocator(0, Long.MAX_VALUE);
+ }
+
+ /**
+ * Get a new child allocator with a default name and a custom initial reservation and maximum allocation.
+ *
+ * @return a new child {@link BufferAllocator}.
+ */
+ public static BufferAllocator getAllocator(final long initReservation, final long maxAllocation) {
+ return ROOT.newChildAllocator(nextChildName(), initReservation, maxAllocation);
+ }
+
+ private static String nextChildName() {
+ return "SingletonAllocator#" + COUNTER.incrementAndGet();
+ }
+
+ /**
+ * Get the total allocated memory across all child allocators.
+ *
+ * @return the number of allocated bytes.
+ */
+ public static long getAllocatedMemory() {
+ return ROOT.getAllocatedMemory();
+ }
+
+ /**
+ * Check if there are any outstanding allocators that have not been closed.
+ *
+ * @return <code>true</code> if child allocators exist or <code>false</code> if not.
+ */
+ public static boolean hasActiveAllocators() {
+ return !ROOT.getChildAllocators().isEmpty();
+ }
+
+ @VisibleForTesting
+ static void checkGlobalCleanUpResources() {
Review Comment:
> Probably this should be public? An application should check that all memory is freed at shutdown
I thought the same, let me expose that also
--
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] davisusanibar commented on a diff in pull request #37178: GH-37177: [Java][Docs] Enable singleton allocator
Posted by "davisusanibar (via GitHub)" <gi...@apache.org>.
davisusanibar commented on code in PR #37178:
URL: https://github.com/apache/arrow/pull/37178#discussion_r1296327949
##########
java/memory/memory-core/src/main/java/org/apache/arrow/memory/util/SingletonAllocator.java:
##########
@@ -0,0 +1,103 @@
+/*
+ * 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.util;
+
+import java.util.concurrent.atomic.AtomicInteger;
+import java.util.stream.Collectors;
+
+import org.apache.arrow.memory.BufferAllocator;
+import org.apache.arrow.memory.RootAllocator;
+import org.apache.arrow.util.VisibleForTesting;
+
+/**
+ * Factory class for managing allocators in simple applications that make
+ * use of an application-wide singleton root allocator with no listeners,
+ * automatic allocator defaults, and no total allocator limit.
Review Comment:
Added
--
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] adamkennedy commented on a diff in pull request #37178: GH-37177: [Java][Docs] Enable singleton allocator
Posted by "adamkennedy (via GitHub)" <gi...@apache.org>.
adamkennedy commented on code in PR #37178:
URL: https://github.com/apache/arrow/pull/37178#discussion_r1296355737
##########
java/memory/memory-core/src/main/java/org/apache/arrow/memory/util/GlobalAllocator.java:
##########
@@ -0,0 +1,99 @@
+/*
+ * 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.util;
+
+import java.util.concurrent.atomic.AtomicInteger;
+import java.util.stream.Collectors;
+
+import org.apache.arrow.memory.BufferAllocator;
+import org.apache.arrow.memory.RootAllocator;
+import org.apache.arrow.util.VisibleForTesting;
+
+/**
+ * Default implementation for memory allocation in case you are using a global
+ * allocator, otherwise, you should pass root allocator explicitly on demand.
+ */
+public final class GlobalAllocator {
+ private GlobalAllocator() {}
+
+ private static final RootAllocator GLOBAL_ROOT_ALLOCATOR = new RootAllocator();
+ private static final AtomicInteger CHILD_NUMBER = new AtomicInteger(0);
+
+ /**
+ * Get default new child allocator.
+ *
+ * @return a new child {@link BufferAllocator}.
+ */
+ public static BufferAllocator getChildAllocator() {
+ return getChildAllocator(0, Long.MAX_VALUE);
+ }
+
+ /**
+ * Get custom new child allocator with initial reservation and maximum allocation.
+ *
+ * @return a new child {@link BufferAllocator}.
+ */
+ public static BufferAllocator getChildAllocator(final long initReservation, final long maxAllocation) {
+ return GLOBAL_ROOT_ALLOCATOR.newChildAllocator(nextChildName(), initReservation, maxAllocation);
+ }
+
+ private static String nextChildName() {
+ return "ChildAllocator-" + CHILD_NUMBER.incrementAndGet();
+ }
+
+ /**
+ * Get the total allocated memory across all child allocators.
+ *
+ * @return the number of allocated bytes.
+ */
+ public static long getAllChildAllocatedMemory() {
+ return GLOBAL_ROOT_ALLOCATOR.getAllocatedMemory();
+ }
+
+ /**
+ * Check allocators that have not been closed.
+ *
+ * @return <code>true</code> if child allocators exist or <code>false</code> if not.
+ */
+ public static boolean hasActiveAllocators() {
+ return !GLOBAL_ROOT_ALLOCATOR.getChildAllocators().isEmpty();
+ }
+
+ @VisibleForTesting
+ static void checkGlobalCleanUpResources() {
Review Comment:
Was this supposed to have been made public?
--
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 #37178: GH-37177: [Java][Doc] Enable singleton allocator
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #37178:
URL: https://github.com/apache/arrow/pull/37178#issuecomment-1679204592
:warning: GitHub issue #37177 **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] adamkennedy commented on a diff in pull request #37178: GH-37177: [Java][Docs] Enable singleton allocator
Posted by "adamkennedy (via GitHub)" <gi...@apache.org>.
adamkennedy commented on code in PR #37178:
URL: https://github.com/apache/arrow/pull/37178#discussion_r1296348047
##########
java/memory/memory-core/src/test/java/org/apache/arrow/memory/util/GlobalAllocatorTest.java:
##########
@@ -0,0 +1,65 @@
+/*
+ * 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.util;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+import org.apache.arrow.memory.BufferAllocator;
+import org.junit.Before;
+import org.junit.Test;
+
+/**
+ * Test cases for {@link GlobalAllocator}.
+ */
+public class GlobalAllocatorTest {
+ @Before
+ public void init() {
+ GlobalAllocator.checkGlobalCleanUpResources();
+ }
+
+ @Test
+ public void testGlobalAllocator() {
+ assertGlobalAllocatorEmpty();
+ try (BufferAllocator allocator = GlobalAllocator.getChildAllocator()) {
+ GlobalAllocator.verifyStateOfAllocator();
+ assertEquals(0, allocator.getAllocatedMemory());
+ assertTrue(allocator.getName().contains("ChildAllocator-"));
Review Comment:
Probably strictly speaking `startsWith`
--
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] davisusanibar commented on a diff in pull request #37178: GH-37177: [Java][Docs] Enable singleton allocator
Posted by "davisusanibar (via GitHub)" <gi...@apache.org>.
davisusanibar commented on code in PR #37178:
URL: https://github.com/apache/arrow/pull/37178#discussion_r1296438970
##########
java/memory/memory-core/src/main/java/org/apache/arrow/memory/util/SingletonAllocator.java:
##########
@@ -0,0 +1,103 @@
+/*
+ * 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.util;
+
+import java.util.concurrent.atomic.AtomicInteger;
+import java.util.stream.Collectors;
+
+import org.apache.arrow.memory.BufferAllocator;
+import org.apache.arrow.memory.RootAllocator;
+import org.apache.arrow.util.VisibleForTesting;
+
+/**
+ * Factory class for managing allocators in simple applications that make
+ * use of an application-wide singleton root allocator with no listeners,
+ * automatic allocator defaults, and no total allocator limit.
+ * <p>
+ * Child allocators are created with a default name with the format
+ * <code>SingletonAllocator#(AtomicCounter)</code>.
+ */
+public final class SingletonAllocator {
Review Comment:
Just filed this issue https://github.com/apache/arrow/issues/37216, please add suggestion/changes as needed.
--
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] davisusanibar commented on pull request #37178: GH-37177: [Java][Docs] Enable singleton allocator
Posted by "davisusanibar (via GitHub)" <gi...@apache.org>.
davisusanibar commented on PR #37178:
URL: https://github.com/apache/arrow/pull/37178#issuecomment-1681283991
I am going to close this issue and incorporate your recommendation into https://github.com/apache/arrow/issues/37216 PRs
--
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] davisusanibar commented on a diff in pull request #37178: GH-37177: [Java][Docs] Enable singleton allocator
Posted by "davisusanibar (via GitHub)" <gi...@apache.org>.
davisusanibar commented on code in PR #37178:
URL: https://github.com/apache/arrow/pull/37178#discussion_r1296273497
##########
java/memory/memory-core/src/main/java/org/apache/arrow/memory/util/SingletonAllocator.java:
##########
@@ -0,0 +1,103 @@
+/*
+ * 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.util;
+
+import java.util.concurrent.atomic.AtomicInteger;
+import java.util.stream.Collectors;
+
+import org.apache.arrow.memory.BufferAllocator;
+import org.apache.arrow.memory.RootAllocator;
+import org.apache.arrow.util.VisibleForTesting;
+
+/**
+ * Factory class for managing allocators in simple applications that make
+ * use of an application-wide singleton root allocator with no listeners,
+ * automatic allocator defaults, and no total allocator limit.
+ * <p>
+ * Child allocators are created with a default name with the format
+ * <code>SingletonAllocator#(AtomicCounter)</code>.
+ */
+public final class SingletonAllocator {
Review Comment:
ohhh ok, then, Does this PR should be closed to add all of this as a proper documentation?
--
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] adamkennedy commented on a diff in pull request #37178: GH-37177: [Java][Docs] Enable singleton allocator
Posted by "adamkennedy (via GitHub)" <gi...@apache.org>.
adamkennedy commented on code in PR #37178:
URL: https://github.com/apache/arrow/pull/37178#discussion_r1296354108
##########
java/memory/memory-core/src/main/java/org/apache/arrow/memory/util/SingletonAllocator.java:
##########
@@ -0,0 +1,103 @@
+/*
+ * 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.util;
+
+import java.util.concurrent.atomic.AtomicInteger;
+import java.util.stream.Collectors;
+
+import org.apache.arrow.memory.BufferAllocator;
+import org.apache.arrow.memory.RootAllocator;
+import org.apache.arrow.util.VisibleForTesting;
+
+/**
+ * Factory class for managing allocators in simple applications that make
+ * use of an application-wide singleton root allocator with no listeners,
+ * automatic allocator defaults, and no total allocator limit.
+ * <p>
+ * Child allocators are created with a default name with the format
+ * <code>SingletonAllocator#(AtomicCounter)</code>.
+ */
+public final class SingletonAllocator {
Review Comment:
@lidavidm I agree that the usage should be banned in `src/main`, but the use of GlobalAllocator in third-party test scripts has uncovered quite a few allocation bugs in Arrow.
We may want to either allow/encourage/mandate the use of GlobalAllocator in the test suite, or provide an additional specialized allocator or factory method for use in the test suite that provides the bug-detecting effects.
--
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 #37178: GH-37177: [Java][Docs] Enable singleton allocator
Posted by "lidavidm (via GitHub)" <gi...@apache.org>.
lidavidm commented on code in PR #37178:
URL: https://github.com/apache/arrow/pull/37178#discussion_r1296363779
##########
java/memory/memory-core/src/main/java/org/apache/arrow/memory/util/SingletonAllocator.java:
##########
@@ -0,0 +1,103 @@
+/*
+ * 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.util;
+
+import java.util.concurrent.atomic.AtomicInteger;
+import java.util.stream.Collectors;
+
+import org.apache.arrow.memory.BufferAllocator;
+import org.apache.arrow.memory.RootAllocator;
+import org.apache.arrow.util.VisibleForTesting;
+
+/**
+ * Factory class for managing allocators in simple applications that make
+ * use of an application-wide singleton root allocator with no listeners,
+ * automatic allocator defaults, and no total allocator limit.
+ * <p>
+ * Child allocators are created with a default name with the format
+ * <code>SingletonAllocator#(AtomicCounter)</code>.
+ */
+public final class SingletonAllocator {
Review Comment:
Yes, the test suite should be closing allocators properly - @davisusanibar can you file an issue to audit the tests? (Possibly, we should define and use a base test class or rule or whatever the JUnit 5 equivalent is.)
--
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