RFR[XS] JDK-8178543 - Optimize Klass::is_shared()

[Jiangli Zhou]

Hi Ioi,

> On Apr 14, 2017, at 5:39 PM, Ioi Lam <ioi.lam at oracle.com> wrote:
> 
> On 4/15/17 2:11 AM, Jiangli Zhou wrote:
>> Hi Ioi,
>> 
>> Thanks for the details. Do you know how big is the increase of ‘dirty’ pages with the specific writes? If the increase of unsharable pages are small then it's okay. It’s a balance between speed and memory.
> 
> Hi Jiangli,
> 
> I ran the start-up benchmark in gdb and set a break point at exit, and check the smaps file:
> 
> BEFORE:
> 
> 801400000-802280000 rw-p 008e4000 fc:01 17360993                         /tmp/iklam/bench2/cds.old.jsa
> Size:              14848 kB
> Rss:               14772 kB
> Pss:               14772 kB
> Shared_Clean:          0 kB
> Shared_Dirty:          0 kB
> Private_Clean:       228 kB
> Private_Dirty:     14544 kB
> Referenced:        14772 kB
> Anonymous:         14544 kB
> 
> AFTER:
> 
> 801400000-802280000 rw-p 008e4000 fc:01 17360995                         /tmp/iklam/bench2/cds.new.jsa
> Size:              14848 kB
> Rss:               14772 kB
> Pss:               14772 kB
> Shared_Clean:          0 kB
> Shared_Dirty:          0 kB
> Private_Clean:       228 kB
> Private_Dirty:     14544 kB
> Referenced:        14772 kB
> Anonymous:         14544 kB
> 
> As you can see, almost the entire RW section is dirty, and there's no difference with or without the change. So I think the change is pretty safe.

Thanks for looking into that. 

> 
>> While looking for the callers of itableMethodEntry::initialize(), I found klassItable::initialize_with_method(). It doesn’t seem to be called by anyone. Could you please also remove that as part of your change if you haven’t integrated it. Otherwise, I’ll file a separate bug.
> 
> Thanks for finding the dead code, I will take that out.

Thanks!

Jiangli

> 
> Thanks
> - Ioi
>> Thanks,
>> Jiangli
>> 
>>> On Apr 13, 2017, at 9:32 PM, Ioi Lam <ioi.lam at oracle.com> wrote:
>>> 
>>> Hi Jiangli,
>>> 
>>> Code like this in klassVtable.cpp is necessary:
>>> 
>>>            if (is_preinitialized_vtable()) {   /// calls _klass->is_shared()
>>>              // At runtime initialize_vtable is rerun for a shared class
>>>              // (loaded by the non-boot loader) as part of link_class_impl().
>>>              // The dumptime vtable index should be the same as the runtime index.
>>>              assert(def_vtable_indices->at(i) == initialized,
>>>                     "dump time vtable index is different from runtime index");
>>>            } else {
>>>              def_vtable_indices->at_put(i, initialized); //set vtable index
>>>            }
>>> 
>>> because the def_vtable_indices for a shared class is in RO space and cannot written into at run time.
>>> 
>>> For this code (before)
>>> 
>>> 1017   if (MetaspaceShared::is_in_shared_space((void*)&_method) &&
>>> 1018      !MetaspaceShared::remapped_readwrite()) {
>>> 1019     // At runtime initialize_itable is rerun as part of link_class_impl()
>>> 1020     // for a shared class loaded by the non-boot loader.
>>> 1021     // The dumptime itable method entry should be the same as the runtime entry.
>>> 1022     assert(_method == m, "sanity");
>>> 1023   } else {
>>> 1024     _method = m;
>>> 1025   }
>>> 1026 }
>>> 
>>> the address (&_method) is either in a vtable or an itable, both of which are in the RO space. Therefore it's OK to write into it.
>>> 
>>> The cost of checking MetaspaceShared::is_in_shared_space((void*)&_method) is high, as at this point we don't readily have a pointer to the corresponding Klass whose i/v table encompasses (&_method).
>>> 
>>> Therefore, I made the assignment unconditional. I kept the assert so we don't lose any functionality.
>>> 
>>> I think the cost of writing into _method is low (compared to the cost of MetaspaceShared::is_in_shared_space) , because it's within an InstanceKlass, which will be written into anyway during InstanceKlass::restore_unshareable_info.
>>> 
>>> AFTER:
>>> 
>>> 1017 #ifdef ASSERT
>>> 1018   if (MetaspaceShared::is_in_shared_space((void*)&_method) &&
>>> 1019      !MetaspaceShared::remapped_readwrite()) {
>>> 1020     // At runtime initialize_itable is rerun as part of link_class_impl()
>>> 1021     // for a shared class loaded by the non-boot loader.
>>> 1022     // The dumptime itable method entry should be the same as the runtime entry.
>>> 1023     assert(_method == m, "sanity");
>>> 1024   }
>>> 1025 #endif
>>> 1026   _method = m;
>>> 1027 }
>>> 
>>> Thanks
>>> - Ioi
>>> 
>>> On 4/14/17 2:02 AM, Jiangli Zhou wrote:
>>>> Hi Ioi,
>>>> 
>>>> Could you please explain the change in klassVtable.cpp?
>>>> 
>>>> Thanks,
>>>> Jiangli
>>>> 
>>>>> On Apr 13, 2017, at 2:30 AM, Ioi Lam <ioi.lam at oracle.com> wrote:
>>>>> 
>>>>> Hi, please review this small start-up enhancement:
>>>>> 
>>>>> https://bugs.openjdk.java.net/browse/JDK-8178543
>>>>> http://cr.openjdk.java.net/~iklam/jdk10/8178543-klass-is-shared.v01/
>>>>> 
>>>>> We have a benchmark that shows Klass::is_shared() is called very frequently during InstanceKlass::link_class_impl, and costs about 2% of the start-up time.
>>>>> 
>>>>> The fix is simple -- instead of walking the list of CDS shared regions, use a new bit in Klass::_access_flags
>>>>> 
>>>>> Thanks
>>>>> - Ioi
>