Add BufferPoolMXBean to track allocated/deallocated pointers

[digiovinazzo]

This PR adds a BufferMXBean to allow tracking of allocation/deallocation of pointers by JavaCPP.

Note: I'm using AtomicLong for the counter as volatile is not enough to ensure exclusive access when incrementing the value using ++. The totalBytes field should also be fixed either synchronizing on DeallocatorThread.class when accessing it or use AtomicLong as well.

[saudet]

It doesn't look like that API is available on Android. Would it be possible to split this out of Pointer, into the tools package I guess? I think it would also make sense to enable this only in a "debug" mode to avoid the overhead, but what do you think?

[digiovinazzo]

Oh, OK, yes it makes sense to have it decoupled from Pointer and only in a debug mode (I guess a system property would work).
It's not clear to me how to do it though: I can move the code to tools package, but then how to activate it? Should I put it in a static initializer, for example the Logger one?

[saudet]

Yeah, you can still refer to that class in the tools directory, that's fine.

[saudet]

Looking good, thanks! Could you elaborate on the need for an atomic? I've been using just volatile for the totalBytes one and it's been working fine. totalCount also only gets updated within synchronized (DeallocatorReference.class) { } blocks so I'm not sure I understand the need for an atomic.

As for the name of the class, we can get rid of the "s", that is PointerBufferPoolMXBean, and as for the name returned by getName(), "javacpp" probably makes more sense there. What do you think?

And the check for the system property should go in Pointer.java with the rest there. Thanks

[saudet]

@netbrain I think this MXBean would fix bytedeco/javacv#1337, what do you think?

[digiovinazzo]

Looking good, thanks! Could you elaborate on the need for an atomic? I've been using just volatile for the totalBytes one and it's been working fine. totalCount also only gets updated within synchronized (DeallocatorReference.class) { } blocks so I'm not sure I understand the need for an atomic.

The volatile keyword is not enough to make atomic operations like ++ or += as they really do "get" and "set" separately.
So when you increment totalBytes += bytes that is not thread-safe just by using volatile, as it really does:

long tmp = totalBytes; // volatile read
// <--- some other thread can enter here
totalBytes = tmp + bytes; // volatile write

So either you need to use a synchronized (DeallocatorReference.class) { } block for totalBytes += bytes; or you use AtomicLong.incrementAndGet()/getAndIncrement().

As for the name of the class, we can get rid of the "s", that is PointerBufferPoolMXBean, and as for the name returned by getName(), "javacpp" probably makes more sense there. What do you think?

And the check for the system property should go in Pointer.java with the rest there. Thanks

OK, applied suggestions.

[saudet]

So either you need to use a synchronized (DeallocatorReference.class) { } block for totalBytes += bytes; or you use AtomicLong.incrementAndGet()/getAndIncrement().

Yeah, that synchronized block is already there. What am I missing?

[digiovinazzo]

Ops, sorry somehow I got wrong here, thinking the write was not inside a synchronized block...

I checked again the code and maybe I have only one doubt : you basically use synchronized (DeallocatorReference.class) { } when you write to totalBytes and volatile when you read its value. So theoretically the read can happen in the middle of the totalBytes += bytes get-and-set operation, but I guess that is not a big deal as when reading you might get the value just before or just after the increment, but that does not affect what happens in the writing thread.

[saudet]

Right, I never had any issues with reading the value. If it's OK, let's remove the atomic. Thanks!

[saudet]

I'm not able to push changes to your repository, but if you could apply this patch, that looks good to merge. Thanks!

diff --git a/src/main/java/org/bytedeco/javacpp/Pointer.java b/src/main/java/org/bytedeco/javacpp/Pointer.java
index f892e63..a17d8af 100644
--- a/src/main/java/org/bytedeco/javacpp/Pointer.java
+++ b/src/main/java/org/bytedeco/javacpp/Pointer.java
@@ -32,7 +32,6 @@ import java.nio.ByteBuffer;
 import java.nio.ByteOrder;
 import java.util.Iterator;
 import java.util.concurrent.atomic.AtomicInteger;
-import java.util.concurrent.atomic.AtomicLong;
 import org.bytedeco.javacpp.annotation.Name;
 import org.bytedeco.javacpp.tools.Generator;
 import org.bytedeco.javacpp.tools.PointerBufferPoolMXBean;
@@ -286,7 +285,7 @@ public class Pointer implements AutoCloseable {
         Deallocator deallocator;
 
         static volatile long totalBytes = 0;
-        static AtomicLong totalCount = new AtomicLong();
+        static volatile long totalCount = 0;
         long bytes;
 
         AtomicInteger count;
@@ -300,7 +299,7 @@ public class Pointer implements AutoCloseable {
                     next.prev = head = this;
                 }
                 totalBytes += bytes;
-                totalCount.incrementAndGet();
+                totalCount++;
             }
         }
 
@@ -319,7 +318,7 @@ public class Pointer implements AutoCloseable {
                 }
                 prev = next = this;
                 totalBytes -= bytes;
-                totalCount.decrementAndGet();
+                totalCount--;
             }
         }
 
@@ -538,7 +537,7 @@ public class Pointer implements AutoCloseable {
 
     /** Returns {@link DeallocatorReference#totalCount}, current number of pointers tracked by deallocators. */
     public static long totalCount() {
-        return DeallocatorReference.totalCount.get();
+        return DeallocatorReference.totalCount;
     }
 
     /** Returns {@link #maxPhysicalBytes}, the maximum amount of physical memory that should be used. */

[matteodg]

Oh, you meant to remove the AtomicLong, not volatile.

But one important thing I was actually thinking: it won't be right either using AtomicLong or volatile because now the values of totalBytes and totalCount have to be consistent, so when reading either one of the two fields now we have to do it inside an synchronized (DeallocatorReference.class) { } block.

[saudet]

That's just for the MXBean, right? We can easily pass a reference to DeallocatorReference.class on register() and have a synchronized block inside there.

[saudet]

FWIW, I don't think consumers of BufferPoolMXBean expect these values to be synchronized. Calls to getTotalCapacity() and getCount() cannot be synchronized from inside the class, it's out of our control.

[matteodg]

I think we just have to make the library right: as totalBytes() and totalCount() are both public methods of Pointer, they have to work when called from any thread: there should be no assumption on consumers' threads, no?
And as they are updated in a synchronized(DeallocatorReference.class) { } block, then they should be read in a similar synchronized block. At that point then volatile can be removed.

Just pushed: now only DeallocatorReference has access to the static fields. Any other class can access the value through getTotalBytes() and getTotalCount(). To change the value I added incrementTotalBytes(), decrementTotalBytes() methods as they are used in the unit tests.

[saudet]

Adding getters with synchronized methods is not going to do anything else than make everything slower. That's why volatile exists! We can use them to make things faster! So please put it back like it was.

[saudet]

Thanks! Like I said, using volatile instead of 2 separate independent synchronized blocks in 2 separate independent method calls that can occur at any undetermined time across multiple threads is going to give the same result. Neither can provide 2 values that are in sync, but the synchronized blocks are a whole lot slower.

[tmm1]

I'm seeing load errors on Android after this change:

12-29 18:19:56.543 19941 19941 I ap: Rejecting re-init on previously-failed class java.lang.Class<org.bytedeco.javacpp.tools.PointerBufferPoolMXBean>: java.lang.NoClassDefFoundError: Failed resolution of: Ljava/lang/management/BufferPoolMXBean;
12-29 18:19:56.543 19941 19941 I ap:   at java.lang.String java.lang.Runtime.nativeLoad(java.lang.String, java.lang.ClassLoader) (Runtime.java:-2)
12-29 18:19:56.543 19941 19941 I ap:   at void java.lang.Runtime.loadLibrary0(java.lang.ClassLoader, java.lang.String) (Runtime.java:1014)
12-29 18:19:56.543 19941 19941 I ap:   at void java.lang.System.loadLibrary(java.lang.String) (System.java:1669)
12-29 18:19:56.543 19941 19941 I ap:   at java.lang.String org.bytedeco.javacpp.Loader.loadLibrary(java.lang.Class, java.net.URL[], java.lang.String, java.lang.String[]) (Loader.java:94)
12-29 18:19:56.543 19941 19941 I ap:   at java.lang.String org.bytedeco.javacpp.Loader.load(java.lang.Class, java.util.Properties, boolean) (Loader.java:89)
12-29 18:19:56.543 19941 19941 I ap:   at java.lang.String org.bytedeco.javacpp.Loader.load() (Loader.java:15)

[saudet]

Those are just warnings though, right? Would you know the "proper" way to make sure Android doesn't try to initialize optional classes like that?

[saudet]

Maybe we should be using reflection to call PointerBufferPoolMXBean.register() here?
https://github.com/bytedeco/javacpp/blob/master/src/main/java/org/bytedeco/javacpp/Pointer.java#L515