RFR 8150607 - Clean up CompactHashtable

[Jiangli Zhou]

Hi Ioi,

I like the more structural way of reading/writing the compact table with SimpleCompactHashtable. It looks quite clean overall.

- How about using VALUE_ONLY_BUCKET_TYPE, which is more descriptive than TINY_BUCKET_TYPE?

- The following assert in CompactSymbolTableWriter::add() limits the total shared space size to be less than MAX_SHARED_DELTA unnecessarily. Symbols are allocated from the RO space at dump time. We only need to make sure the max delta between the ‘base_address’ and the end of RO space is less than MAX_SHARED_DELTA. This is not a new issue introduced by your change, you don’t have to address that as part of this change if you prefer. I’ll file a separate RFE.

 171 void CompactSymbolTableWriter::add(unsigned int hash, Symbol *symbol) {
 172   address base_address = address(MetaspaceShared::shared_rs()->base());
 173   uintx max_delta = uintx(MetaspaceShared::shared_rs()->size());
 174   assert(max_delta <= MAX_SHARED_DELTA, "range check");

- Why is the default RO space size increased?

- src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/memory/SymbolTable.java
- src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/utilities/CompactHashTable.java
  Copyright year.

Thanks,
Jiangli

> On Mar 31, 2016, at 10:43 PM, Ioi Lam <ioi.lam at oracle.com> wrote:
> 
> Please review
> 
> http://cr.openjdk.java.net/~iklam/jdk9/8150607_cleanup_compact_hashtable.v01/
> 
> Bug: Clean up CompactHashtable
> 
>    https://bugs.openjdk.java.net/browse/JDK-8150607
> 
> Summary of fix:
> 
> [1] Instead of reading/writing the table bit-by-bit, which is tedious and
>    error prone, use SimpleCompactHashtable::serialize(), which is more
>    structural.
> 
> [2] Split up the _buckets and _entries into two separate arrays, so the
>    dumping and walking code is easier to understand
> 
>    (see comments above SimpleCompactHashtable declaration)
>    These 2 arrays are now allocated from the RO region (used to be in RW)
> 
> [3] Renamed a few things
> 
>    COMPACT_BUCKET_TYPE -> TINY_BUCKET_TYPE
>        (having something called "compact" in CompactHashtable is confusing)
> 
>    The old names "dump_table" (actually dumping the buckets) and
>    "dump_buckets" (actually dumping the entries) were conflicting with
>    terminology used elsewhere. Now the terminology is unified:
>        "buckets" = the main index, "entries" = the second level.
> 
>    lookup_entry -> decode_entry (it wasn't doing any lookup)
> 
> [4] Changed all "*p++" to "array->at_put", so out-of-bounds can be
>    checked with assert.
> 
> [5] Replaced all juint to u4 (suggested by Coleen)
> 
> [6] templatize the iterator (see CompactHashtable::symbols_do ->
>    SimpleCompactHashtable::iterate)
> 
> [7] I also added a test case using Serviceability Agent -- due to the lack of
>    a regression test, the walking of the compact hashtable in SA had been
>    broken for a while before it was fixed in JDK-8151368, so having a test
>    case would make the code more maintainable.
> 
> Tests:
> 
>    Hotspot JTREG tests
> 
> Thanks
> - Ioi