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