RFR (L) 8171392 Move Klass pointers outside of ConstantPool entries so ConstantPool can be read-only
Lois Foltan
lois.foltan at oracle.com
Mon Apr 17 12:41:23 UTC 2017
On 4/16/2017 5:33 AM, Ioi Lam wrote:
> Hi Lois,
>
> I have updated the patch to include your comments, and fixes the
> handling of anonymous classes. I also added some more comments
> regarding the _temp_resolved_klass_index:
Reviewed, looks good! Thank you for making those changes.
Lois
>
> (delta from last webrev)
> http://cr.openjdk.java.net/~iklam/jdk10/8171392_make_constantpool_read_only.v03.delta/
>
>
> (full webrev)
> http://cr.openjdk.java.net/~iklam/jdk10/8171392_make_constantpool_read_only.v03/
>
>
> Thanks
> - Ioi
>
> On 4/15/17 2:31 AM, Lois Foltan wrote:
>> On 4/14/2017 11:30 AM, Ioi Lam wrote:
>>>
>>>
>>> On 4/14/17 1:31 PM, Ioi Lam wrote:
>>>> HI Lois,
>>>>
>>>> Thanks for the review. Please see my comments in-line.
>>>>
>>>> On 4/14/17 4:32 AM, Lois Foltan wrote:
>>>>> Hi Ioi,
>>>>>
>>>>> Looks really good. A couple of comments:
>>>>>
>>>>> src/share/vm/classfile/classFileParser.cpp:
>>>>> * line #5676 - I'm not sure I completely understand the logic
>>>>> surrounding anonymous classes. Coleen and I discussed earlier
>>>>> today and I came away from that discussion with the idea that the
>>>>> only classes being patched currently are anonymous classes.
>>>> Line 5676 ...
>>>>
>>>> 5676 if (is_anonymous()) {
>>>> 5677 _max_num_patched_klasses ++; // for patching the <this>
>>>> class index
>>>> 5678 }
>>>>
>>>> corresponds to
>>>>
>>>> 5361 ik->set_name(_class_name);
>>>> 5362
>>>> 5363 if (is_anonymous()) {
>>>> 5364 // I am well known to myself
>>>> 5365 patch_class(ik->constants(), _this_class_index, ik,
>>>> ik->name()); // eagerly resolve
>>>> 5366 }
>>>>
>>>> Even though the class is "anonymous", it actually has a name.
>>>> ik->name() probably is part of the constant pool, but I am not 100%
>>>> sure. Also, I would need to search the constant pool to find the
>>>> index for ik->name(). So I just got lazy here and use the same
>>>> logic in ConstantPool::patch_class() to append ik->name() to the
>>>> end of the constant pool.
>>>>
>>>> "Anonymous" actually means "the class cannot be looked up by name
>>>> in the SystemDictionary". I think we need a better terminology :-)
>>>>
>>>
>>> I finally realized why we need the "eagerly resolve" on line 5365.
>>> I'll modify the comments to the following:
>>>
>>> // _this_class_index is a CONSTANT_Class entry that refers to this
>>> // anonymous class itself. If this class needs to refer to its
>>> own methods or
>>> // fields, it would use a CONSTANT_MethodRef, etc, which would
>>> reference
>>> // _this_class_index. However, because this class is anonymous
>>> (it's
>>> // not stored in SystemDictionary), _this_class_index cannot be
>>> resolved
>>> // with ConstantPool::klass_at_impl, which does a
>>> SystemDictionary lookup.
>>> // Therefore, we must eagerly resolve _this_class_index now.
>>>
>>> So, Lois is right. Line 5676 is not necessary. I will revise the
>>> code to do the "eager resolution" without using
>>> ClassFileParser::patch_class. I'll post the updated code later.
>>
>> Thanks Ioi for studying this and explaining! Look forward to seeing
>> the updated webrev.
>> Lois
>>
>>>
>>> Thanks
>>> - Ioi
>>>
>>>>> So a bit confused as why the check on line #5676 and a check for a
>>>>> java/lang/Class on line #5684.
>>>> 5683 Handle patch = cp_patch_at(i);
>>>> 5684 if (java_lang_String::is_instance(patch()) ||
>>>> java_lang_Class::is_instance(patch())) {
>>>> 5685 // We need to append the names of the patched
>>>> classes to the end of the constant pool,
>>>> 5686 // because a patched class may have a Utf8 name
>>>> that's not already included in the
>>>> 5687 // original constant pool.
>>>> 5688 //
>>>> 5689 // Note that a String in cp_patch_at(i) may be used
>>>> to patch a Utf8, a String, or a Class.
>>>> 5690 // At this point, we don't know the tag for index i
>>>> yet, because we haven't parsed the
>>>> 5691 // constant pool. So we can only assume the worst --
>>>> every String is used to patch a Class.
>>>> 5692 _max_num_patched_klasses ++;
>>>>
>>>> Line 5684 checks for all objects in the cp_patch array. Later, when
>>>> ClassFileParser::patch_constant_pool() is called, any objects that
>>>> are either Class or String could be treated as a Klass:
>>>>
>>>> 724 void ClassFileParser::patch_constant_pool(ConstantPool* cp,
>>>> 725 int index,
>>>> 726 Handle patch,
>>>> 727 TRAPS) {
>>>> ...
>>>> 732 switch (cp->tag_at(index).value()) {
>>>> 733
>>>> 734 case JVM_CONSTANT_UnresolvedClass: {
>>>> 735 // Patching a class means pre-resolving it.
>>>> 736 // The name in the constant pool is ignored.
>>>> 737 if (java_lang_Class::is_instance(patch())) {
>>>> 738 guarantee_property(!java_lang_Class::is_primitive(patch()),
>>>> 739 "Illegal class patch at %d in class
>>>> file %s",
>>>> 740 index, CHECK);
>>>> 741 Klass* k = java_lang_Class::as_Klass(patch());
>>>> 742 patch_class(cp, index, k, k->name());
>>>> 743 } else {
>>>> 744 guarantee_property(java_lang_String::is_instance(patch()),
>>>> 745 "Illegal class patch at %d in class
>>>> file %s",
>>>> 746 index, CHECK);
>>>> 747 Symbol* const name =
>>>> java_lang_String::as_symbol(patch(), CHECK);
>>>> 748 patch_class(cp, index, NULL, name);
>>>> 749 }
>>>> 750 break;
>>>> 751 }
>>>>
>>>>> Could the is_anonymous() if statement be combined into the loop?
>>>>
>>>> I think the answer is no. At line 5365, there is no guarantee that
>>>> ik->name() is in the cp_patch array.
>>>>
>>>> 5365 patch_class(ik->constants(), _this_class_index, ik,
>>>> ik->name()); // eagerly resolve
>>>>
>>>>> Also why not do this calculation in the rewriter or is that too
>>>>> late?
>>>>>
>>>> Line 5676 and 5684 need to be executed BEFORE the constant pool and
>>>> the associated tags array is allocated (both of which are fixed
>>>> size, and cannot be expanded), which is way before the rewriter is
>>>> run. At this point, we don't know what cp->tag_at(index) is (line
>>>> #732), so the code needs to make a worst-case estimate on how long
>>>> the CP/tags should be.
>>>>
>>>>> * line #5677, 5692 - a nit but I think the convention is to not
>>>>> have a space after the variable name and between the post
>>>>> increment operator.
>>>>>
>>>> Fixed.
>>>>> src/share/vm/classfile/constantPool.hpp:
>>>>> I understand the concept behind _invalid_resolved_klass_index, but
>>>>> it really is not so much invalid as temporary for class
>>>>> redefinition purposes, as you explain in
>>>>> ConstantPool::allocate_resolved_klasses. Please consider renaming
>>>>> to _temp_unresolved_klass_index. And whether you choose to rename
>>>>> the field or not, please add a one line comment ahead of
>>>>> ConstantPool::temp_unresolved_klass_at_put that only class
>>>>> redefinition uses this currently.
>>>>>
>>>> Good idea. Will do.
>>>>
>>>> Thanks
>>>> - Ioi
>>>>
>>>>> Great change, thanks!
>>>>> Lois
>>>>>
>>>>> On 4/13/2017 4:56 AM, Ioi Lam wrote:
>>>>>> Hi Coleen,
>>>>>>
>>>>>> Thanks for the comments. Here's a delta from the last patch
>>>>>>
>>>>>> http://cr.openjdk.java.net/~iklam/jdk10/8171392_make_constantpool_read_only.v02/
>>>>>>
>>>>>>
>>>>>> In addition to your requests, I made these changes:
>>>>>>
>>>>>> [1] To consolidate the multiple extract_high/low code, I've added
>>>>>> CPKlassSlot, so the code is cleaner:
>>>>>>
>>>>>> CPKlassSlot kslot = this_cp->klass_slot_at(which);
>>>>>> int resolved_klass_index = kslot.resolved_klass_index();
>>>>>> int name_index = kslot.name_index();
>>>>>>
>>>>>> [2] Renamed ConstantPool::is_shared_quick() to
>>>>>> ConstantPool::is_shared(). The C++ compiler should be able to
>>>>>> pick this function over MetaspaceObj::is_shared().
>>>>>>
>>>>>> [3] Massaged the CDS region size set-up code a little to pass
>>>>>> internal tests, because RO/RW ratio has changed. I didn't spend
>>>>>> too much time picking the "right" sizes, as this code will be
>>>>>> obsoleted soon with JDK-8072061
>>>>>>
>>>>>> Thanks
>>>>>> - Ioi
>>>>>>
>>>>>> On 4/13/17 6:40 AM, coleen.phillimore at oracle.com wrote:
>>>>>>>
>>>>>>> This looks really good!
>>>>>>>
>>>>>>> http://cr.openjdk.java.net/~iklam/jdk10/8171392_make_constantpool_read_only.v01/src/share/vm/oops/constantPool.cpp.udiff.html
>>>>>>>
>>>>>>>
>>>>>>> + // Add one extra element to tags for storing
>>>>>>> ConstantPool::flags().
>>>>>>> + Array<u1>* tags =
>>>>>>> MetadataFactory::new_writeable_array<u1>(loader_data, length+1,
>>>>>>> 0, CHECK_NULL); ... + assert(tags->length()-1 == _length,
>>>>>>> "invariant"); // tags->at(_length) is flags()
>>>>>>>
>>>>>>>
>>>>>>> I think this is left over, since _flags didn't get moved after all.
>>>>>>>
>>>>>>> + Klass** adr =
>>>>>>> this_cp->resolved_klasses()->adr_at(resolved_klass_index);
>>>>>>> + OrderAccess::release_store_ptr((Klass* volatile *)adr, k);
>>>>>>> + // The interpreter assumes when the tag is stored, the klass
>>>>>>> is resolved
>>>>>>> + // and the Klass* is a klass rather than a Symbol*, so we need
>>>>>>> + // hardware store ordering here.
>>>>>>> + this_cp->release_tag_at_put(which, JVM_CONSTANT_Class);
>>>>>>> + return k;
>>>>>>>
>>>>>>> The comment still refers to the switch between Symbol* and
>>>>>>> Klass* in the constant pool. The entry in the Klass array
>>>>>>> should be NULL.
>>>>>>>
>>>>>>> + int name_index =
>>>>>>> extract_high_short_from_int(*int_at_addr(which));
>>>>>>>
>>>>>>> Can you put klass_name_index_at() in the constantPool.hpp header
>>>>>>> file (so it's inlined) and have all the places where you get
>>>>>>> name_index use this function? So we don't have to know in
>>>>>>> multiple places that extract_high_short_from_int() is where the
>>>>>>> name index is. And in constantPool.hpp, for
>>>>>>> unresolved_klass_at_put() add a comment about what the new
>>>>>>> format of the constant pool entry is {name_index,
>>>>>>> resolved_klass_index}. I'm happy to see this work nearing
>>>>>>> completion! The "constant" pool should be constant! thanks, Coleen
>>>>>>> On 4/11/17 2:26 AM, Ioi Lam wrote:
>>>>>>>> Hi,please review the following change
>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8171392
>>>>>>>> http://cr.openjdk.java.net/~iklam/jdk10/8171392_make_constantpool_read_only.v01/
>>>>>>>> *Summary:** * Before: + ConstantPool::klass_at(i) finds the
>>>>>>>> Klass from the i-th slot of ConstantPool. + When a klass is
>>>>>>>> resolved, the ConstantPool is modified to store the Klass
>>>>>>>> pointer. After: + ConstantPool::klass_at(i) finds the at
>>>>>>>> this->_resolved_klasses->at(i) + When a klass is resolved,
>>>>>>>> _resolved_klasses->at(i) is modified. In addition: + I
>>>>>>>> moved _resolved_references and _reference_map from
>>>>>>>> ConstantPool to ConstantPoolCache. + Now _flags is no
>>>>>>>> longer modified for shared ConstantPools. As a result, none
>>>>>>>> of the fields in shared ConstantPools are modified at run time,
>>>>>>>> so we can move them into the RO region in the CDS archive.
>>>>>>>> *Testing:** * - Benchmark results show no performance
>>>>>>>> regression, despite the extra level of indirection, which has a
>>>>>>>> negligible overhead for the interpreter. - RBT testing for
>>>>>>>> tier2 and tier3. *Ports:** * - I have tested only the
>>>>>>>> Oracle-support ports. For the aarch64, ppc and s390 ports, I
>>>>>>>> have added some code without testing (it's probably
>>>>>>>> incomplete) - Port owners, please check if my patch work for
>>>>>>>> you, and I can incorporate your changes in my push.
>>>>>>>> Alternatively, you can wait for my push and provide fixes (if
>>>>>>>> necessary) in a new changeset, and I will be happy to sponsor
>>>>>>>> it. Thanks - Ioi
>>>>>>
>>>>>
>>>>
>>>
>>
>
More information about the hotspot-dev
mailing list