RFR (L) 8171392 Move Klass pointers outside of ConstantPool entries so ConstantPool can be read-only
Volker Simonis
volker.simonis at gmail.com
Fri Apr 21 13:32:23 UTC 2017
Thanks for looking at the change Martin!
On Fri, Apr 21, 2017 at 3:29 PM, Doerr, Martin <martin.doerr at sap.com> wrote:
> Hi,
>
> the PPC64 (big and little Endian) and s390x parts look correct. Thanks for porting them, Volker.
>
> On PPC64, the memory barrier is very tricky, but I have convinced myself that it's correct.
> The _resolved_klasses array gets allocated during initialization and just the tag gets patched after class resolution.
> So it is sufficient to order only the load of the tag wrt. the load of the Klass* which is exactly what you have implemented.
>
> @Ioi: I think it would be nice to have a little more comments at all places which use release_tag... or release_store_ptr and the respective acquire functions.
> It takes quite some time to find out what exactly needs to be ordered with what, where are possible races etc.
> But it's just on my wish list, not a must have for your change. Thanks also from my side for taking care of our platforms.
>
> Best regards,
> Martin
>
>
> -----Original Message-----
> From: Schmidt, Lutz
> Sent: Donnerstag, 20. April 2017 23:24
> To: Volker Simonis <volker.simonis at gmail.com>
> Cc: Ioi Lam <ioi.lam at oracle.com>; Doerr, Martin <martin.doerr at sap.com>; Lois Foltan <lois.foltan at oracle.com>; HotSpot Open Source Developers <hotspot-dev at openjdk.java.net>
> Subject: Re: RFR (L) 8171392 Move Klass pointers outside of ConstantPool entries so ConstantPool can be read-only
>
> Hi Volker,
>
> you additions are ok for me - they are as good as it gets when you add pointer indirections. My ok statement holds for s390x in particular, but ppc looks good as well. Let’s see if Martin has extended comments on ppc.
>
> Regards, Lutz
>
>
>> On 20 Apr 2017, at 18:02, Volker Simonis <volker.simonis at gmail.com> wrote:
>>
>> Hi Ioi,
>>
>> thanks once again for considering our ports! Please find the required
>> additions for ppc64/s390x in the following webrew (which is based upon
>> your latest v03 patch):
>>
>> http://cr.openjdk.java.net/~simonis/webrevs/2017/8171392_ppc64_s390x/
>>
>> @Martin/@Lucy: could you please have a look at my ppc64/s390x assembly
>> code. I did some tests and I think it should be correct, but maybe you
>> still find some improvements :)
>>
>> Besides that, I have some general questions/comments regarding your change:
>>
>> 1. In constantPool.hpp, why don't you declare the '_name_index' and
>> '_resolved_klass_index' fields with type 'jushort'? As far as I can
>> see, they can only hold 16-bit values anyway. It would also save you
>> some space and several asserts (e.g. in unresolved_klass_at_put():
>>
>>
>> 274 assert((name_index & 0xffff0000) == 0, "must be");
>> 275 assert((resolved_klass_index & 0xffff0000) == 0, "must be");
>>
>> 2. What do you mean by:
>>
>> 106 // ... will be changed to support compressed pointers
>> 107 Array<Klass*>* _resolved_klasses;
>>
>> 3. Why don't we need the call to "release_tag_at_put()" in
>> "klass_at_put(int class_index, Klass* k)"? "klass_at_put(int
>> class_index, Klass* k)" is used from
>> "ClassFileParser::fill_instance_klass() and before your change that
>> function used the previous version of "klass_at_put(int class_index,
>> Klass* k)" which did call "release_tag_at_put()".
>>
>> 4. In ConstantPool::copy_entry_to() you've changed the behavior for
>> tags JVM_CONSTANT_Class, JVM_CONSTANT_UnresolvedClass,
>> JVM_CONSTANT_UnresolvedClassInError. Before, the resolved klass was
>> copied to the new constant pool if one existed but now you always only
>> copy a class_index to the new constant pool (even if a resolved klass
>> existed). Is that OK? E.g. won't this lead to a new resolving for the
>> new constant pool and will this have performance impacts or other side
>> effects?
>>
>> Thanks again for doing this nice change and best regards,
>> Volker
>>
>>
>> On Sun, Apr 16, 2017 at 11:33 AM, Ioi Lam <ioi.lam at oracle.com> 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:
>>>
>>> (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