RFR (L) 8171392 Move Klass pointers outside of ConstantPool entries so ConstantPool can be read-only
Volker Simonis
volker.simonis at gmail.com
Wed May 3 14:39:29 UTC 2017
Fine with me!
Regards,
Volker
On Wed, May 3, 2017 at 3:02 PM, Ioi Lam <ioi.lam at oracle.com> wrote:
> Andrew replied me off-list that he tested the aarch64 part and was happy
> about it. Thanks Andrew.
>
> So if there if no further comment, I will push the code as is.
>
> Thanks
> - Ioi
>
>
> On 4/29/17 8:30 AM, Ioi Lam wrote:
>>
>> 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