[9, P1] RFR (S/M): 8180048: Interned string and symbol table leak memory during parallel unlinking

[coleen.phillimore at oracle.com]

Thomas,

This looks good.  It's a bit hard to understand quickly.   A couple of 
comments would help.

http://cr.openjdk.java.net/~tschatzl/8180048/webrev.1/src/share/vm/utilities/hashtable.cpp.udiff.html

Add a comment to bulk_free_entries before the cas like:

// Add thread local list of removed entries in removed_head to the head 
of the table's _free_list thread safely.

It would help that _removed_head and _removed_tail (and the other 
fields) have leading underscores as is the convention of nonstatic data 
members, so it's easy to see these are the fields of 
BucketUnlinkContext. I think accessor functions are unnecessary though.

http://cr.openjdk.java.net/~tschatzl/8180048/webrev.1/src/share/vm/classfile/stringTable.cpp.udiff.html
http://cr.openjdk.java.net/~tschatzl/8180048/webrev.1/src/share/vm/classfile/symbolTable.cpp.udiff.html

Say for BucketUnlinkContext a short reminder why we need this here.

// unlink_or_oops do is called by multiple threads so needs to hold 
deleted elements in a thread local context to bulk delete

(or something like this)

If you change this, and add a some comment, I don't require a new 
webrev.  This looks like a good change.

Thanks,
Coleen

On 5/12/17 6:46 AM, Thomas Schatzl wrote:
> Hi again,
>
> On Thu, 2017-05-11 at 18:04 +0200, Thomas Schatzl wrote:
>> Hi all,
>>
>>    can I have reviews for this change that fixes a memory leak when
>> unlinking (removing) elements from the string and symbol tables in
>> parallel?
>>
>> [...]
>>
>> I am also going to try to get this change into 8u.
>>
>> Ideally I would also like to have a reviewer from the runtime team
>> (cc'ed).
>>
>> CR:
>> https://bugs.openjdk.java.net/browse/JDK-8180048
>> Webrev:
>> http://cr.openjdk.java.net/~tschatzl/8180048/webrev/
>> Testing:
>> jprt, manual testing
>    I got some comments from Erik Helin, with the following webrev as
> result:
>
> http://cr.openjdk.java.net/~tschatzl/8180048/webrev.0_to_1/ (diff)
> http://cr.openjdk.java.net/~tschatzl/8180048/webrev.1/ (full)
>
> Testing:
> jprt
>
> Thanks,
>    Thomas
>