RFR (L) 8171392 Move Klass pointers outside of ConstantPool entries so ConstantPool can be read-only
Ioi Lam
ioi.lam at oracle.com
Sat Apr 29 15:30:06 UTC 2017
I've updated the patch to include Volker's ppc/s390 port as well as his
comments. I've also included an updated patch (untested) for aarch64 for
Andrew Haley to test:
Full patch
http://cr.openjdk.java.net/~iklam/jdk10/8171392_make_constantpool_read_only.v04/
Delta from the previous version
http://cr.openjdk.java.net/~iklam/jdk10/8171392_make_constantpool_read_only.v04.delta/
Thanks
- Ioi
On 4/28/17 4:30 AM, Ioi Lam wrote:
>
>
> On 4/25/17 8:06 AM, Volker Simonis wrote:
>> On Mon, Apr 24, 2017 at 2:18 PM, Ioi Lam <ioi.lam at oracle.com> wrote:
>>> Hi Volker,
>>>
>>>
>>> On 4/21/17 12:02 AM, Volker Simonis 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/
>>> Thanks for the patch. I will integrate it and post an updated webrev.
>>>> @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");
>>>
>>> I think the HotSpot convention is to use ints as parameter and
>>> return types,
>>> for values that are actually 16-bits or less, like here in
>>> constantPool.hpp:
>>>
>>> void field_at_put(int which, int class_index, int
>>> name_and_type_index) {
>>> tag_at_put(which, JVM_CONSTANT_Fieldref);
>>> *int_at_addr(which) = ((jint) name_and_type_index<<16) |
>>> class_index;
>>> }
>>>
>>> I am not sure what the reasons are. It could be that the parameters
>>> usually
>>> need to be computed arithmetically, and it's much easier for the
>>> caller of
>>> the method to use ints -- otherwise you will get lots of compiler
>>> warnings
>>> which would force you to use lots of casting, resulting in code
>>> that's hard
>>> to read and probably incorrect.
>>>
>> OK, but you could still use shorts in the the object to save space,
>> although I'm not sure how much that will save in total. But if nobody
>> else cares, I'm fine with the current version.
>
> The CPKlassSlot objects are stored only on the stack, so the savings
> is not worth the trouble of adding extract type casts.
>
> Inside the ConstantPool itself, the name_index and
> resolved_klass_index are stored as a pair of 16-bit values.
>
>>>> 2. What do you mean by:
>>>>
>>>> 106 // ... will be changed to support compressed pointers
>>>> 107 Array<Klass*>* _resolved_klasses;
>>>
>>> Sorry the comment isn't very clear. How about this?
>>>
>>> 106 // Consider using an array of compressed klass pointers to
>>> // save space on 64-bit platforms.
>>> 107 Array<Klass*>* _resolved_klasses;
>>>
>> Sorry I still didn't get it? Do you mean you want to use array of
>> "narrowKlass" (i.e. unsigned int)? But using compressed class pointers
>> is a runtime decision while this is a compile time decision.
>
> I haven't figured out how to do it yet :-)
>
> Most likely, it will be something like:
>
> union {
> Array<Klass*>* X;
> Array<narrowKlass>* Y;
> } _resolved_klasses;
>
> and you need to decide at run time whether to use X or Y.
>
> - Ioi
>>>> 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()".
>>>
>>> Good catch. I'll add the following, because the class is now resolved.
>>>
>>> release_tag_at_put(class_index, JVM_CONSTANT_UnresolvedClass);
>>>> 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?
>>> I think Coleen has answered this in a separate mail :-)
>>>
>>> Thanks
>>> - Ioi
>>>
>>>> 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