RFR 8150607 - Clean up CompactHashtable

[Ioi Lam]

Hi Jiangli,


Thanks for the review:

On 4/11/16 6:44 PM, Jiangli Zhou wrote:
> 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?
>
OK, I will make the change.

> - 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.
>
I think it's better to do this in a separate RFE since MAX_SHARED_DELTA 
is used elsewhere as well.

>   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?
>
That's because the symbol table is moved from RW to RO, so I had to 
change the RO minimum size, or else 
test/runtime/SharedArchiveFile/LimitSharedSizes.java would fail.
> - 
> 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.

Will fix.

Thanks
- Ioi
>
> Thanks,
> Jiangli
>
>> On Mar 31, 2016, at 10:43 PM, Ioi Lam <ioi.lam at oracle.com 
>> <mailto:ioi.lam at oracle.com>> wrote:
>>
>> Please review
>>
>> http://cr.openjdk.java.net/~iklam/jdk9/8150607_cleanup_compact_hashtable.v01/ 
>> <http://cr.openjdk.java.net/%7Eiklam/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
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/serviceability-dev/attachments/20160411/a60e7ecf/attachment-0001.html>