RFR (L) 8171392 Move Klass pointers outside of ConstantPool entries so ConstantPool can be read-only
Ioi Lam
ioi.lam at oracle.com
Sun Apr 16 09:33:03 UTC 2017
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:
(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