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