RFR(S) 8210388 Use hash table to store archived subgraph_info records (original) (raw)

Ioi Lam ioi.lam at oracle.com
Wed Oct 3 02:05:05 UTC 2018

Hi Jiangli,

Thanks for the suggestion. I will add it.

I'll wait for the default CDS to be integrated before pushing my changes.

Thanks

On 10/2/18 6:43 PM, Jiangli Zhou wrote:

Hi Ioi,

The update looks good. Please see one minor suggestion below. The default CDS archive is moving along and is expected to be integrated in the next few days (if no other surprises). To avoid any delay of the default CDS archive integration, it would be better to temporary postpone all other non-test related CDS/AppCDS changes (if they are non-trivial). Please let me know if that works for you. - src/hotspot/share/memory/heapShared.cpp  174   bool doentry(Klass* klass, KlassSubGraphInfo& info) {  175     if (info.subgraphobjectklasses() != NULL || info.subgraphentryfields() != NULL) { Please add an assert to make sure if info.subgraphentryfields() is not NULL when info.subgraphobjectklasses() != NULL. If no subgraph from any entry field is archived for a containing klass, the info should not have any subgraphobjectklasses. Thanks, Jiangli

On 10/2/18 9:23 AM, Ioi Lam wrote: Hi Jiangli,

Thanks for the review. The updated webrev: http://cr.openjdk.java.net/~iklam/jdk12/8210388-hashtable-subgraph-info.v02.delta/

http://cr.openjdk.java.net/~iklam/jdk12/8210388-hashtable-subgraph-info.v02.full/

On 9/21/18 3:49 PM, Jiangli Zhou wrote: Hi Ioi, The change looks clean. Here are my comments: - src/hotspot/share/classfile/symbolTable.cpp  654     if (fixedhash == 0) {  655       return true;  656     } It's been a long time since I touched this code. Can you please remind me when the result of hashsharedsymbol can be 0? hashsharedsymbol eventually calls this: static unsigned int hashcode(const jchar* s, int len) { unsigned int h = 0; while (len-- > 0) { h = 31*h + (unsigned int) *s; s++; } return h; } So the empty symbol has a hashcode of 0. Valid class/field/method names and signatures cannot be empty, so I think we are safe there. Otherwise it's pretty easy to come up with symbols that can overflow "h" to zero. I wrote a program to find a few of them: { 4 25 31 19 29 24 4 } => 4294967296 { 4 25 31 19 29 23 35 } => 4294967296 { 4 25 31 19 29 22 66 } => 4294967296 { 4 25 31 19 29 21 97 } => 4294967296 { 4 25 31 19 29 20 128 } => 4294967296 { 4 25 31 19 29 19 159 } => 4294967296 { 4 25 31 19 29 18 190 } => 4294967296 { 4 25 31 19 29 17 221 } => 4294967296 { 4 25 31 19 29 16 252 } => 4294967296 But I don't see any such symbols while dumping the default CDS archive, which has 36163 symbols. - src/hotspot/share/memory/heapShared.cpp 52 KlassSubGraphInfo* HeapShared::getsubgraphinfo(Klass* k) { Since you are here, can you please add an assert to make sure it's called at dump time only. Fixed. 175 if (info.subgraphobjectklasses() != NULL || info.subgraphentryfields() != NULL) { We should not have a subgraphinfo with no objectklasses and no entry field. A subgraphinfo can have no objectklasses, but should have at least one entry field. I think we should have an assert instead. Sometimes no fields can be archived (e.g., cacheObject/ArchivedIntegerCacheTest.java which tries to archive a very large array). So I'll leave this check there. 202 runtimesubgraphinfotable.reset(); Why is the above needed in HeapShared::createhashtables()? I removed it and added an assert for !DumpSharedSpaces in HeapShared::initializefromarchivedsubgraph. HeapShared::createhashtables() can use a better name. How about HeapShared::writearchivedsubgraphinfotable()? HeapShared::serializehashtables() can also be renamed. Changed: HeapShared::createhashtables() -> HeapShared::writesubgraphinfotable() HeapShared::serializehashtables() -> HeapShared::serializesubgraphinfotableheader() I also renamed the following functions to be clear what they are doing: SymbolTable::serialize -> SymbolTable::serializesharedtableheader StringTable::serialize -> StringTable::serializesharedtableheader 282 logdebug(cds, heap)("  %p init field @ %2d = %p", k, fieldoffset, (address)v); It's better to use PTRFORMAT. Fixed - src/hotspot/share/memory/heapShared.hpp 120   class DumpTimeKlassSubGraphInfoTable  121     : public ResourceHashtable<Klass*, KlassSubGraphInfo,_  _122                                HeapShared::klasshash,_  _123                                HeapShared::klassequals,_  _124                                15889, // prime number_  _125                                ResourceObj::CHEAP> {  126   public:  127     int count;  128   }; A much smaller size can be used. Currently, we only have single digit subgraph infos. In the future, it might grow to hundreds infos. I changed to 137 for now. I might increase it later when I add lambda for Lambdas. Thanks - Ioi Thanks, Jiangli On 9/20/18 4:46 PM, Jiangli Zhou wrote: Hi Ioi, On 9/20/18 4:41 PM, Ioi Lam wrote:

On 09/20/2018 03:07 PM, Jiangli Zhou wrote: Hi Ioi, On 9/20/18 11:28 AM, Ioi Lam wrote: http://cr.openjdk.java.net/~iklam/jdk12/8210388-hashtable-subgraph-info.v01/ https://bugs.openjdk.java.net/browse/JDK-8210388 Here's another small step towards CDS support for Lambda (JDK-8198698). Replace the linear search of ArchivedKlassSubGraphInfoRecord with a hashtable. Currently we have just 6 such records, but with Lambda support that number is going to increase substiantially. Shared subgraphs table stats Number of entries       :         6 Total bytes used        :        72 Average bytes per entry :    12.000 Average bucket size     :     6.000 Variance of bucket size :     0.000 Std. dev. of bucket size:     0.000 Empty buckets           :         0 ValueOnly buckets      :         0 Other buckets           :         1 I also took the chance to clean up the use of CompactHashtableWriter in the symbol/string tables. In doing so, I fixed a bug where SymbolTable::copysharedsymboltable was skipping symbols with zero hashcodes. This should be done for the string table only. Jiangli, could you help me with this comments? unsigned int hash = javalangString::hashcode(s); if (hash == 0) { // We do not archive Strings with a 0 hashcode because ...... return true; } The intent was to prevent possible write into the closed archive region when identityhash was called on an empty String. The empty String is archived in the open archive heap region. This may not be necessary as we always pre-compute identity hash for archived object. If you want to remove this restriction, please be careful to verify all possible cases. Hi Jiangli, Thanks for the explanation. I am not going to change the logic in the code quoted above. I just wanted to add a comment there to explain why we are doing that. Do we have an existing comment for this else where? Please do. Thanks. There is no existing comment, I think. I removed the (hash == 0) check in the symbol table dumping code, since that's a bug. I'll take a look of your change. Thanks, Jiangli Thanks - Ioi Thanks, Jiangli

Thanks - Ioi

More information about the hotspot-runtime-dev mailing list