On Thu, 20 Mar 2025 22:35:23 GMT, Brent Christian <bchri...@openjdk.org> wrote:
> I propose some cleanups to `FinalizerHistogramTest.java` to hopefully clear
> up the intermittent failures:
>
> * run with `othervm`: this test blocks the (global) finalizer thread, and
> also requires the (global) finalizer thread to enter the test's `finalize()`
> method
> * The test uses `volatile` ints, but sets them based on their current value,
> which is not reliable; convert to `AtomicInteger`
> * use `PhantomReference`s to ensure that at least two `MyObject`s have become
> unreachable. If one is stuck in `finalize()`, at least one is still waiting
> to be finalized and should show up in the histogram.
test/jdk/java/lang/ref/FinalizerHistogramTest.java line 38:
> 36: * @modules java.base/java.lang.ref:open
> 37: * @library /test/lib
> 38: * @build jdk.test.lib.util.ForceGC
I don't think `@build` is doing anything here
test/jdk/java/lang/ref/FinalizerHistogramTest.java line 45:
> 43: static ReentrantLock lock = new ReentrantLock();
> 44: static final AtomicInteger wasInitialized = new AtomicInteger(0);
> 45: static final AtomicInteger wasTrapped = new AtomicInteger(0);
Minor: Do you think the name might be confusing? May be naming `wasInitialized`
and `wasTrapped` as `initialisedCount` and `trappedCount` might be more
descriptive, what do you think?
test/jdk/java/lang/ref/FinalizerHistogramTest.java line 72:
> 70: // GC and wait for at least 2 MyObjects to be ready for
> finalization,
> 71: // and one MyObject to be stuck in finalize().
> 72: ForceGC.wait(() -> { return ref1.refersTo(null) &&
Nitpick: I think you don't need a { return ....} here, it can be a pure lambda
afaik. But if you prefer this way, I don't mind in the slightest 😃
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/24143#discussion_r2007697872
PR Review Comment: https://git.openjdk.org/jdk/pull/24143#discussion_r2007697994
PR Review Comment: https://git.openjdk.org/jdk/pull/24143#discussion_r2007698178
