RFR 8150607 - Clean up CompactHashtable

[David Holmes]

On 18/04/2016 12:32 AM, Ioi Lam wrote:
> Hi Calvin,
>
> Thanks for the review. Please see my comments inside.
>
> On 4/13/16 1:03 PM, Calvin Cheung wrote:
>> Hi Ioi,
>>
>> Just a few minor comments. No need to see another webrev.
>> compactHashtable.cpp:
>>
>> 71   _num_entries ++;
>>
>> extra space before ++
>> similarly in lines 80, 112, 123, 125
>>
> I grepped the hotspot source code with '[a-z] [+][+]' and found that
> there are quite a few cases of putting a space between the variable name
> and "++". Other lines in compactHashtable.cpp that are not changed by
> this patch also use "<space>++". The HotSpot coding guide
> (https://wiki.openjdk.java.net/display/HotSpot/StyleGuide) doesn't
> mention the "++" operator specifically, so I'll leave the space there
> for now.

Then we need to update the style guide - unary operators (like the 
booleans the guide does mention) should not have a space between the 
operator and operand.

Cheers,
David
-----


>
>> 118 Entry tent = bucket->at(i);
>>
>> It is clearer if 'tent' is just 'ent' since the code in this block is
>> not dealing with tiny entry.
>>
> Changed tent -> ent
>
>> 249 while (entry <entry_max) {
>>
>> Needs a space before entry_max.
>>
> Fixed.
>>
>> SASymbolTableTest.java:
>>
>> The result of p.getPid() can be saved into a local variable so that it
>> won't need to be done twice in lines 86 and 93.
>>
> Fixed.
>
> Thanks
> - Ioi
>> thanks,
>> Calvin
>>
>> On 4/12/16, 12:40 PM, Jiangli Zhou wrote:
>>> Ioi,
>>>
>>>> On Apr 11, 2016, at 7:11 PM, Ioi Lam<ioi.lam at oracle.com>  wrote:
>>>>
>>>> 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.
>>> I filed JDK-8154108<https://bugs.openjdk.java.net/browse/JDK-8154108>.
>>>
>>>>>   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.
>>> Ok.
>>>
>>> Thanks,
>>> Jiangli
>>>
>>>>> -
>>>>> 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<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
>