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....@oracle.com
<mailto:ioi....@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
