RFR (L) 8171392 Move Klass pointers outside of ConstantPool entries so ConstantPool can be read-only
Ioi Lam
ioi.lam at oracle.com
Fri Apr 14 15:30:22 UTC 2017
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
>> 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