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

Thomas Schatzl thomas.schatzl at oracle.com
Mon May 15 10:22:31 UTC 2017


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



More information about the hotspot-runtime-dev mailing list