RFR (L) 8171392 Move Klass pointers outside of ConstantPool entries so ConstantPool can be read-only
Lois Foltan
lois.foltan at oracle.com
Fri Apr 14 18:31:07 UTC 2017
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