Quantcast

RFR (L) 8171392 Move Klass pointers outside of ConstantPool entries so ConstantPool can be read-only

classic Classic list List threaded Threaded
19 messages Options
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

RFR (L) 8171392 Move Klass pointers outside of ConstantPool entries so ConstantPool can be read-only

Ioi Lam
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

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR (L) 8171392 Move Klass pointers outside of ConstantPool entries so ConstantPool can be read-only

Volker Simonis
Hi Ioi,

thanks for considering our ports. At least ppc64 and s390 require some
additions tough :)
I'm currently working on them and will provide the full ppc64/s390 versions
shortly.

Regards,
Volker


On Tue, Apr 11, 2017 at 8:26 AM, Ioi Lam <[hidden email]> wrote:

> Hi,please review the following change
>
> https://bugs.openjdk.java.net/browse/JDK-8171392
> http://cr.openjdk.java.net/~iklam/jdk10/8171392_make_constan
> tpool_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
>
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR (L) 8171392 Move Klass pointers outside of ConstantPool entries so ConstantPool can be read-only

coleen.phillimore
In reply to this post by Ioi Lam

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
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR (L) 8171392 Move Klass pointers outside of ConstantPool entries so ConstantPool can be read-only

Ioi Lam
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, [hidden email] 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

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR (L) 8171392 Move Klass pointers outside of ConstantPool entries so ConstantPool can be read-only

Lois Foltan
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.  So a bit confused as why
the check on line #5676 and a check for a java/lang/Class on line
#5684.  Could the is_anonymous() if statement be combined into the
loop?  Also why not do this calculation in the rewriter or is that too late?

* 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.

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.

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, [hidden email] 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
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR (L) 8171392 Move Klass pointers outside of ConstantPool entries so ConstantPool can be read-only

coleen.phillimore
In reply to this post by Ioi Lam

Hi Ioi,
The changes look good.  Thanks for doing klass_slot_at() and I agree
is_shared is better than is_shared_quick.
thanks,
Coleen

On 4/13/17 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, [hidden email] 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
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR (L) 8171392 Move Klass pointers outside of ConstantPool entries so ConstantPool can be read-only

Ioi Lam
In reply to this post by Lois Foltan
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 :-)

> 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, [hidden email] 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
>>
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR (L) 8171392 Move Klass pointers outside of ConstantPool entries so ConstantPool can be read-only

Ioi Lam


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, [hidden email] 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
>>>
>>
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR (L) 8171392 Move Klass pointers outside of ConstantPool entries so ConstantPool can be read-only

Lois Foltan
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, [hidden email] 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
>>>>
>>>
>>
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR (L) 8171392 Move Klass pointers outside of ConstantPool entries so ConstantPool can be read-only

Ioi Lam
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, [hidden email] 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
>>>>>
>>>>
>>>
>>
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR (L) 8171392 Move Klass pointers outside of ConstantPool entries so ConstantPool can be read-only

Lois Foltan

On 4/16/2017 5:33 AM, Ioi Lam 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:

Reviewed, looks good!  Thank you for making those changes.
Lois

>
> (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, [hidden email] 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
>>>>>>
>>>>>
>>>>
>>>
>>
>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR (L) 8171392 Move Klass pointers outside of ConstantPool entries so ConstantPool can be read-only

Volker Simonis
In reply to this post by Ioi Lam
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 <[hidden email]> 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, [hidden email] 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
>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR (L) 8171392 Move Klass pointers outside of ConstantPool entries so ConstantPool can be read-only

Schmidt, Lutz
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 <[hidden email]> 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 <[hidden email]> 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, [hidden email] 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
>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR (L) 8171392 Move Klass pointers outside of ConstantPool entries so ConstantPool can be read-only

Volker Simonis
Thanks, Lutz!

On Thu, Apr 20, 2017 at 11:24 PM, Schmidt, Lutz <[hidden email]> wrote:

> 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 <[hidden email]> 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 <[hidden email]> 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, [hidden email] 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
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

RE: RFR (L) 8171392 Move Klass pointers outside of ConstantPool entries so ConstantPool can be read-only

Doerr, Martin
In reply to this post by Schmidt, Lutz
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 <[hidden email]>
Cc: Ioi Lam <[hidden email]>; Doerr, Martin <[hidden email]>; Lois Foltan <[hidden email]>; HotSpot Open Source Developers <[hidden email]>
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 <[hidden email]> 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 <[hidden email]> 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, [hidden email] 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
>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR (L) 8171392 Move Klass pointers outside of ConstantPool entries so ConstantPool can be read-only

Volker Simonis
Thanks for looking at the change Martin!


On Fri, Apr 21, 2017 at 3:29 PM, Doerr, Martin <[hidden email]> 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 <[hidden email]>
> Cc: Ioi Lam <[hidden email]>; Doerr, Martin <[hidden email]>; Lois Foltan <[hidden email]>; HotSpot Open Source Developers <[hidden email]>
> 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 <[hidden email]> 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 <[hidden email]> 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, [hidden email] 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
>>>>>>>>
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>
Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR (L) 8171392 Move Klass pointers outside of ConstantPool entries so ConstantPool can be read-only

coleen.phillimore
In reply to this post by Volker Simonis


On 4/20/17 5:02 PM, 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/
>
> @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?

For redefinition, I don't think there would be a noticeable performance
impact to leaving the class in the unresolved state in the new constant
pool.  The interpreter calls into the vm for ldc <class> anyway each
time (should file a bug and fix that), and if the class is loaded it
will find it in an (unlocked, I believe) system dictionary lookup.  The
checkcast and instanceof cases might be more expensive though.

If redefinition performance becomes an issue again for customers, we can
consider optimizing this by copying the class over to the new constant
pool klasses array but we're trying to move away from the constant pool
merging code long term.   We have other redefinition enhancements that
we're considering for jdk11.

thanks,
Coleen

>
> Thanks again for doing this nice change and best regards,
> Volker
>
>
> On Sun, Apr 16, 2017 at 11:33 AM, Ioi Lam <[hidden email]> 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, [hidden email] 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
>>>>>>>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR (L) 8171392 Move Klass pointers outside of ConstantPool entries so ConstantPool can be read-only

Ioi Lam
In reply to this post by Volker Simonis
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.

> 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;

> 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 <[hidden email]> 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, [hidden email] 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
>>>>>>>

Reply | Threaded
Open this post in threaded view
|  
Report Content as Inappropriate

Re: RFR (L) 8171392 Move Klass pointers outside of ConstantPool entries so ConstantPool can be read-only

Volker Simonis
On Mon, Apr 24, 2017 at 2:18 PM, Ioi Lam <[hidden email]> 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.

>> 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.

>> 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 <[hidden email]> 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, [hidden email] 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
>>>>>>>>
>>>>>>>>
>
Loading...