lidavidm commented on code in PR #37498:
URL: https://github.com/apache/arrow/pull/37498#discussion_r1315177784
##########
docs/source/java/memory.rst:
##########
@@ -288,6 +288,49 @@ Finally, enabling the ``TRACE`` logging level will
automatically provide this st
| at RootAllocator.close (RootAllocator.java:29)
| at (#8:1)
+A further option would be to change the way how allocators are created, we
could try the following:
+
+1. Set up a global root allocator that is going to be used in each module,
+2. Create child allocators by using the root allocator,
+3. Give child allocations proper names so that you can detect errors in
another class or method,
+4. Ensure that resources are properly closed,
+5. At some strategic point, check if the global allocator is empty,
+6. In case of error, try to review the above-identified class/method allocator
bugs.
+
+.. code-block:: java
+
+ //1
+ private static final RootAllocator allocator = new RootAllocator();
Review Comment:
```suggestion
private static final BufferAllocator allocator = new RootAllocator();
```
You generally should never declare RootAllocator as the type of a variable
or parameter.
##########
docs/source/java/memory.rst:
##########
@@ -288,6 +288,49 @@ Finally, enabling the ``TRACE`` logging level will
automatically provide this st
| at RootAllocator.close (RootAllocator.java:29)
| at (#8:1)
+A further option would be to change the way how allocators are created, we
could try the following:
+
+1. Set up a global root allocator that is going to be used in each module,
+2. Create child allocators by using the root allocator,
+3. Give child allocations proper names so that you can detect errors in
another class or method,
+4. Ensure that resources are properly closed,
+5. At some strategic point, check if the global allocator is empty,
+6. In case of error, try to review the above-identified class/method allocator
bugs.
+
+.. code-block:: java
+
+ //1
+ private static final RootAllocator allocator = new RootAllocator();
+ ...
+ //2
+ public static BufferAllocator getChildAllocator() {
+ return allocator.newChildAllocator(nextChildName(), 0,
Long.MAX_VALUEn);
+ }
+ ...
+ //3
+ private static String nextChildName() {
+ return "Allocator-Child-" + getClassNameAndMethodName();
Review Comment:
`getClassNameAndMethodName` isn't defined
##########
docs/source/java/memory.rst:
##########
@@ -288,6 +288,49 @@ Finally, enabling the ``TRACE`` logging level will
automatically provide this st
| at RootAllocator.close (RootAllocator.java:29)
| at (#8:1)
+A further option would be to change the way how allocators are created, we
could try the following:
+
+1. Set up a global root allocator that is going to be used in each module,
+2. Create child allocators by using the root allocator,
+3. Give child allocations proper names so that you can detect errors in
another class or method,
+4. Ensure that resources are properly closed,
+5. At some strategic point, check if the global allocator is empty,
+6. In case of error, try to review the above-identified class/method allocator
bugs.
Review Comment:
```suggestion
Sometimes, explicitly passing allocators around is difficult. For example, it
can be hard to pass around extra state, like an allocator, through layers of
existing application or framework code. A global or singleton allocator
instance
can be useful here, though it should not be your first choice.
How this works:
1. Set up a global allocator in a singleton class.
2. Provide methods to create child allocators from the global allocator.
3. Give child allocators proper names to make it easier to figure out where
allocations occurred in case of errors.
4. Ensure that resources are properly closed.
5. Check that the global allocator is empty at some suitable point, such as
right before program shutdown.
6. If it is not empty, review the above allocation bugs.
```
##########
docs/source/java/memory.rst:
##########
@@ -288,6 +288,49 @@ Finally, enabling the ``TRACE`` logging level will
automatically provide this st
| at RootAllocator.close (RootAllocator.java:29)
| at (#8:1)
+A further option would be to change the way how allocators are created, we
could try the following:
+
+1. Set up a global root allocator that is going to be used in each module,
+2. Create child allocators by using the root allocator,
+3. Give child allocations proper names so that you can detect errors in
another class or method,
+4. Ensure that resources are properly closed,
+5. At some strategic point, check if the global allocator is empty,
+6. In case of error, try to review the above-identified class/method allocator
bugs.
+
+.. code-block:: java
+
+ //1
+ private static final RootAllocator allocator = new RootAllocator();
+ ...
+ //2
+ public static BufferAllocator getChildAllocator() {
+ return allocator.newChildAllocator(nextChildName(), 0,
Long.MAX_VALUEn);
+ }
+ ...
+ //3
+ private static String nextChildName() {
+ return "Allocator-Child-" + getClassNameAndMethodName();
+ }
+ ...
+ //4: Business code
+ try () {
+ ...
+ }
+ ...
+ //5
+ public static void checkGlobalCleanUpResources() {
+ ...
+ !allocator.getChildAllocators().isEmpty();
+ allocator.getAllocatedMemory() != 0;
+ ...
+ throw new IllegalStateException(...);
+ }
+ ...
+ //6: Allocator bug detected
+ Review active allocators on: Allocator-Child-com.yourpackage.Test-method
Review Comment:
Remove this?
##########
docs/source/java/memory.rst:
##########
@@ -288,6 +288,49 @@ Finally, enabling the ``TRACE`` logging level will
automatically provide this st
| at RootAllocator.close (RootAllocator.java:29)
| at (#8:1)
+A further option would be to change the way how allocators are created, we
could try the following:
+
+1. Set up a global root allocator that is going to be used in each module,
+2. Create child allocators by using the root allocator,
+3. Give child allocations proper names so that you can detect errors in
another class or method,
+4. Ensure that resources are properly closed,
+5. At some strategic point, check if the global allocator is empty,
+6. In case of error, try to review the above-identified class/method allocator
bugs.
+
+.. code-block:: java
+
+ //1
+ private static final RootAllocator allocator = new RootAllocator();
+ ...
+ //2
+ public static BufferAllocator getChildAllocator() {
+ return allocator.newChildAllocator(nextChildName(), 0,
Long.MAX_VALUEn);
+ }
+ ...
+ //3
+ private static String nextChildName() {
+ return "Allocator-Child-" + getClassNameAndMethodName();
+ }
+ ...
+ //4: Business code
+ try () {
Review Comment:
```suggestion
try (BufferAllocator allocator = GlobalAllocator.getChildAllocator()) {
```
##########
docs/source/java/memory.rst:
##########
@@ -288,6 +288,49 @@ Finally, enabling the ``TRACE`` logging level will
automatically provide this st
| at RootAllocator.close (RootAllocator.java:29)
| at (#8:1)
+A further option would be to change the way how allocators are created, we
could try the following:
+
+1. Set up a global root allocator that is going to be used in each module,
+2. Create child allocators by using the root allocator,
+3. Give child allocations proper names so that you can detect errors in
another class or method,
+4. Ensure that resources are properly closed,
+5. At some strategic point, check if the global allocator is empty,
+6. In case of error, try to review the above-identified class/method allocator
bugs.
+
+.. code-block:: java
+
+ //1
+ private static final RootAllocator allocator = new RootAllocator();
+ ...
+ //2
+ public static BufferAllocator getChildAllocator() {
+ return allocator.newChildAllocator(nextChildName(), 0,
Long.MAX_VALUEn);
Review Comment:
```suggestion
return allocator.newChildAllocator(nextChildName(), 0,
Long.MAX_VALUE);
```
##########
docs/source/java/memory.rst:
##########
@@ -288,6 +288,49 @@ Finally, enabling the ``TRACE`` logging level will
automatically provide this st
| at RootAllocator.close (RootAllocator.java:29)
| at (#8:1)
+A further option would be to change the way how allocators are created, we
could try the following:
+
+1. Set up a global root allocator that is going to be used in each module,
+2. Create child allocators by using the root allocator,
+3. Give child allocations proper names so that you can detect errors in
another class or method,
+4. Ensure that resources are properly closed,
+5. At some strategic point, check if the global allocator is empty,
+6. In case of error, try to review the above-identified class/method allocator
bugs.
+
+.. code-block:: java
+
+ //1
+ private static final RootAllocator allocator = new RootAllocator();
+ ...
+ //2
+ public static BufferAllocator getChildAllocator() {
+ return allocator.newChildAllocator(nextChildName(), 0,
Long.MAX_VALUEn);
+ }
+ ...
+ //3
+ private static String nextChildName() {
+ return "Allocator-Child-" + getClassNameAndMethodName();
+ }
+ ...
+ //4: Business code
+ try () {
+ ...
+ }
+ ...
+ //5
+ public static void checkGlobalCleanUpResources() {
+ ...
+ !allocator.getChildAllocators().isEmpty();
+ allocator.getAllocatedMemory() != 0;
+ ...
+ throw new IllegalStateException(...);
Review Comment:
Wrap this in an `if (...) { throw ...; }`
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
