code review request: Test case for JDK-7042126 HashMap.clone() memory leak
peter.levart at gmail.com
Thu Jan 31 13:47:39 UTC 2013
You could combine the B and C in the following pattern:
- Create a WeakReference to the observed object;
- Execute the potentially "leaky" operation;
- Trigger gc with after check of WeakReference. Maybe a couple of times.
- If that does not clear the reference, fill-in heap until OOME is
thrown and after that check the reference.
On 01/31/2013 10:40 AM, David Buck wrote:
> I was curious to see what others have done in the past and took a look
> at about 15 different testcases for memory leaks in the jdk tree and
> basically found 3 patterns:
> Pattern A: Use a loop that would exercise the relevant code for a
> finite number of runs. If an OOME is not thrown, the test passes. In
> order for the test to finish quickly, most people seem to specify a
> very small Xmx to limit the number of iterations needed. (example:
> Pattern B: Code similar to what I have submitted for this review that
> tries to leak one object, explicitly triggers a GC event, and then
> confirms that the WeakReference was nulled out. One thing to note is
> that in every case I found of this the author chose to run multiple GC
> events, usually somewhere between 5 to 10000 iterations. (example:
> Pattern C: These cases used a WeakReference like Pattern B above, but
> instead of depending on an explicit call to System.gc(), would
> intentionally fill the heap until an OOME is thrown to trigger at
> least 1 "full" gc event. (example:
> Patterns A and B were most common (with pattern B being slightly more
> common), and I only found Pattern C 2 times
> Strictly speaking, all 3 patters are problematic as they each depend
> on undefined behavior in some way or another. Patterns A and C require
> the JVM to handle OOMEs somewhat gracefully (Pattern A when we fail,
> Pattern C when we pass), and pattern B requires an explicit call to
> System.gc to collect an arbitrary object.
> The fact that we do not routinely see false positives from these cases
> indicates that in practice, they all seem to work OK for the time being.
> After thinking about this for a bit, I actually really like pattern C.
> Even though the JVM is not technically guaranteed to gracefully handle
> all OOME conditions, we try very hard to be as robust as possible in
> the face of Java-heap OOME. Again, the fact that we do not routinely
> see false positives from these tests proves that in practice, this
> works very well. Even if an otherwise "correct" change (say in the
> JVM) resulted in Pattern C suddenly failing, it is something we would
> almost certainly want to investigate as it would indicate that the
> robustness of the JVM has somehow regressed.
> I plan to modify my testcase to follow pattern C and resubmit for
> review. If anyone has any other ideas or comments, I would be grateful
> for your input.
> On 2013/01/30 22:32, David Buck wrote:
>> Hi Alan!
>> Thank you for your help.
>> TL;DR version:
>> Strictly speaking (going only by the specifications), this could give us
>> false positives, but I believe this is VERY unlikely to actually happen
>> in real life.
>> Long version:
>> Yes, I gave this some thought myself. For example, on JRockit, if the
>> object were in old space and System.gc() only did a young collection
>> (the default behavior for JRockit), this test would result in a false
>> positive. In fact, as the JVM is allowed by the specification to
>> completely ignore explicit GC calls, we could never guarantee that we
>> would the WeakReference would always get nulled out.
>> That said, in pactice this works very well for both HotSpot and JRockit.
>> Every scenario I have tried it out on (with both JVMs) has provided the
>> expected result every single time (i.e. failing when expected; never
>> resulting in false positive otherwise). It seems that both of Oracle's
>> JVMs as currently implemented are very unlikely to run into any issues
>> here. Marking the test cases as "othervm" also helps to remove most edge
>> case scenarios where you could still somehow imagine this failing. (For
>> example, on a JRockit-like JVM, other tests running concurrently could
>> trigger a gc in the middle of this test resulting in the HashMap and its
>> contents being promoted to old space and the null reference not being
>> cleared during the call to System.gc() as expected.)
>> One option would be to mark this as a manually-run test if we wanted to
>> be extra cautious. What do you think?
>> > Minor nit, should be WeakReference<Object> to avoid the raw type.
>> I will update the webrev once we have decided what (if anything) to do
>> regarding the risk of false positives.
>> On 2013/01/30 22:09, Alan Bateman wrote:
>>> On 29/01/2013 23:36, David Buck wrote:
>>>> This is a review request to add only the test case for the following
>>>> OracleJDK issue :
>>>> [ 7042126 : (alt-rt) HashMap.clone implementation should be
>>>> re-examined ]
>>>> * please note: I just marked the bug as "public" this morning, so
>>>> there will be a short delay before the above link is available.
>>>> The issue (root cause) is not in OpenJDK (i.e. the problem was
>>>> OracleJDK specific), but the test case is valid for any Java SE
>>>> implementation so it should go into OpenJDK so we can prevent a
>>>> similar issue from ever happening in both releases moving forward. The
>>>> test case simply helps ensure that the contents of a HashMap are not
>>>> leaked when we clone the HashMap.
>>>> [ Code Review for jdk ]
>>> How robust is this test? I'm concerned that it might fail
>>> if the System.gc doesn't immediately GC the now un-references
>>> entries in
>>> Minor nit, should be WeakReference<Object> to avoid the raw type.
More information about the core-libs-dev