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

Ioi Lam ioi.lam at oracle.com
Fri Apr 14 05:31:33 UTC 2017

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);
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()) {
  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(), 
  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.
> 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.

- Ioi

> Great change, thanks!
> Lois
> On 4/13/2017 4:56 AM, Ioi Lam wrote:
>> Hi Coleen,
>> Thanks for the comments. Here's a delta from the last patch
>> http://cr.openjdk.java.net/~iklam/jdk10/8171392_make_constantpool_read_only.v02/ 
>> In addition to your requests, I made these changes:
>> [1] To consolidate the multiple extract_high/low code, I've added 
>> CPKlassSlot, so the code is cleaner:
>>   CPKlassSlot kslot = this_cp->klass_slot_at(which);
>>   int resolved_klass_index = kslot.resolved_klass_index();
>>   int name_index = kslot.name_index();
>> [2] Renamed ConstantPool::is_shared_quick() to 
>> ConstantPool::is_shared(). The C++ compiler should be able to pick 
>> this function over MetaspaceObj::is_shared().
>> [3] Massaged the CDS region size set-up code a little to pass 
>> internal tests, because RO/RW ratio has changed. I didn't spend too 
>> much time picking the "right" sizes, as this code will be obsoleted 
>> soon with JDK-8072061
>> Thanks
>> - Ioi
>> On 4/13/17 6:40 AM, coleen.phillimore at oracle.com wrote:
>>> This looks really good!
>>> http://cr.openjdk.java.net/~iklam/jdk10/8171392_make_constantpool_read_only.v01/src/share/vm/oops/constantPool.cpp.udiff.html 
>>> + // Add one extra element to tags for storing ConstantPool::flags().
>>> + Array<u1>* tags = 
>>> MetadataFactory::new_writeable_array<u1>(loader_data, length+1, 0, 
>>> CHECK_NULL); ... + assert(tags->length()-1 == _length, "invariant"); 
>>> // tags->at(_length) is flags()
>>> I think this is left over, since _flags didn't get moved after all.
>>> + Klass** adr = 
>>> this_cp->resolved_klasses()->adr_at(resolved_klass_index);
>>> + OrderAccess::release_store_ptr((Klass* volatile *)adr, k);
>>> + // The interpreter assumes when the tag is stored, the klass is 
>>> resolved
>>> + // and the Klass* is a klass rather than a Symbol*, so we need
>>> + // hardware store ordering here.
>>> + this_cp->release_tag_at_put(which, JVM_CONSTANT_Class);
>>> + return k;
>>> The comment still refers to the switch between Symbol* and Klass* in 
>>> the constant pool.  The entry in the Klass array should be NULL.
>>> + int name_index = extract_high_short_from_int(*int_at_addr(which));
>>> Can you put klass_name_index_at() in the constantPool.hpp header 
>>> file (so it's inlined) and have all the places where you get 
>>> name_index use this function?  So we don't have to know in multiple 
>>> places that extract_high_short_from_int() is where the name index 
>>> is. And in constantPool.hpp, for unresolved_klass_at_put() add a 
>>> comment about what the new format of the constant pool entry is 
>>> {name_index, resolved_klass_index}. I'm happy to see this work 
>>> nearing completion!  The "constant" pool should be constant! thanks, 
>>> Coleen
>>> On 4/11/17 2:26 AM, Ioi Lam wrote:
>>>> Hi,please review the following change 
>>>> https://bugs.openjdk.java.net/browse/JDK-8171392 
>>>> http://cr.openjdk.java.net/~iklam/jdk10/8171392_make_constantpool_read_only.v01/ 
>>>> *Summary:** *   Before:   + ConstantPool::klass_at(i) finds the 
>>>> Klass from the i-th slot of ConstantPool.   + When a klass is 
>>>> resolved, the ConstantPool is modified to store the Klass 
>>>> pointer.   After:   + ConstantPool::klass_at(i) finds the at 
>>>> this->_resolved_klasses->at(i)   + When a klass is resolved, 
>>>> _resolved_klasses->at(i) is modified.   In addition:     + I moved 
>>>> _resolved_references and _reference_map from ConstantPool       to 
>>>> ConstantPoolCache.     + Now _flags is no longer modified for 
>>>> shared ConstantPools.   As a result, none of the fields in shared 
>>>> ConstantPools are modified at run time,   so we can move them into 
>>>> the RO region in the CDS archive. *Testing:** *   - Benchmark 
>>>> results show no performance regression, despite the extra level 
>>>> of     indirection, which has a negligible overhead for the 
>>>> interpreter.   - RBT testing for tier2 and tier3. *Ports:** *   - I 
>>>> have tested only the Oracle-support ports. For the aarch64, ppc and 
>>>> s390 ports, I have added some code without testing (it's probably 
>>>> incomplete)   - Port owners, please check if my patch work for you, 
>>>> and I can incorporate your     changes in my push. Alternatively, 
>>>> you can wait for my push and provide fixes (if necessary) in a new 
>>>> changeset, and I will be happy to sponsor it. Thanks - Ioi 

More information about the hotspot-dev mailing list