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

[coleen.phillimore at oracle.com]

Yes, I like your additional comments.
Thanks,
Coleen

On 5/15/17 6:22 AM, Thomas Schatzl wrote:
> Hi Coleen,
>
>    thanks a lot for your review. :)
>
> On Fri, 2017-05-12 at 19:47 -0400, coleen.phillimore at oracle.com wrote:
>> 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/ut
>> ilities/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.
> I tried to improve the comments in the changed areas to better indicate
> why the change uses the BucketUnlinkContext.
>
>> 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.
> Fixed.
>
>> http://cr.openjdk.java.net/~tschatzl/8180048/webrev.1/src/share/vm/cl
>> assfile/stringTable.cpp.udiff.html
>> http://cr.openjdk.java.net/~tschatzl/8180048/webrev.1/src/share/vm/cl
>> assfile/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.
> For reference:
> http://cr.openjdk.java.net/~tschatzl/8180048/webrev.1_to_2/ (diff)
> http://cr.openjdk.java.net/~tschatzl/8180048/webrev.2/ (full)
>
> Thanks,
>    Thomas
>