RFR(M) 8011621: Keep the "live range id - node" mapping in a separate class

[Niclas Adlertz]

Hi again,

Thank you for your comments.

Vladimir:
---------

> ${JAVA_HOME}/bin/java -server -Xss4m -verify -XX:+CompileTheWorld -XX:+TimeCompiler -Xbootclasspath/p:${JAVA_HOME}/jre/lib/rt.jar

Base: Total compilation: 2235.880 sec.
Fix: Total compilation: 2226.020 sec.

So the fix should be ok in terms of compilation time. 

> Can you use full name LiveRangeMap instead of LRGMap?
Sure, fixed.

> Don't put big methods with loops into header file, we don't want to increase code size without need.
Ok, fixed.

> Explicitly declare private: and public: parts in class. Add comment what is class for.
Done.

> I like Roland's suggestion (LRGMap _lrg_map;) because it avoids a reference through _lrg_map pointer (in compiled code C++ needs only one PhaseChaitin* pointer).
Done.

Roland:
-------

> In vmStructs.cpp, you need to add a declaration for PhaseChaitin::_lrg_map otherwise:
> c2_nonstatic_field(LRGMap,             _max_lrg_id,              uint)
> is useless.
I removed it from vmStructs.cpp completely instead, since it's not used by the agent.

> Why not declare: LRGMap _lrg_map; rather than stack allocate it and set a pointer.
I just followed the old pattern, but that's a good remark, thank you.
Fixed.

> Can you add comments describing what LRGMap is before its declaration?
> 
> Shouldn't that:
> 242   if (C->unique() > _lrg_map->_names.Size()) {
> 243     _lrg_map->_names.extend(C->unique() - 1, 0);
> 244   }
> 
> be moved to the LRGMap constructor?

This code is not necessary anymore, because I also took the liberty of removing 
pd_preallocate_hook(); 
and 
pd_postallocate_verify_hook();

which are not needed anymore because they only added nodes for Win95 and Win 98.

> Shouldn't you make _max_lrg_id, _uf_map and _names private and have accessors?
I have now made public methods for performing operations on _max_lrg_id, _uf_map and _names.

---------

The new webrev: http://cr.openjdk.java.net/~adlertz/JDK-8011621/webrev02/

Thank you.

Regards,
Niclas Adlertz

On 8 apr 2013, at 20:34, Vladimir Kozlov wrote:

> Niclas,
> 
> Thank you for doing this cleanup.
> 
> Please, test affection on RA performance. You can use CTW for this and fastdebug VM:
> 
> ${JAVA_HOME}/bin/java -server -Xss4m -verify -XX:+CompileTheWorld -XX:+TimeCompiler -Xbootclasspath/p:${JAVA_HOME}/jre/lib/rt.jar
> 
> Or build optimized VM and run jvm08, for example, with -XX:+TimeCompiler.
> 
> Can you use full name LiveRangeMap instead of LRGMap?
> Don't put big methods with loops into header file, we don't want to increase code size without need.
> 
> Explicitly declare private: and public: parts in class. Add comment what is class for.
> 
> I like Roland's suggestion (LRGMap _lrg_map;) because it avoids a reference through _lrg_map pointer (in compiled code C++ needs only one PhaseChaitin* pointer).
> 
> Thanks,
> Vladimir
> 
> On 4/8/13 3:05 AM, Niclas Adlertz wrote:
>> Hi all.
>> 
>> Problem:
>> In order to clean up the Split code (in opto/reg_split.cpp) making it easier to maintain and (hopefully) making it easier to improve (https://jbs.oracle.com/bugs/browse/JDK-7022320), it would be good to have the Split code more isolated with as few dependencies to other parts of the code as possible.
>> The Split code could reside in an own module (class), just having references to classes and data it really needs. Right now it would need to have a reference to PhaseChaitin, only to access the "live range id - node" mapping. However, it does not need to know about everything in the PhaseChaitin class. This may also introduce cross reference problems since PhaseChaitin would use the Split module and the Split module would need a reference to PhaseChaitin.
>> 
>> Solution:
>> Move the "live range id - node" mapping to a separate class, instantiated by PhaseChaitin, which can then be passed to the Split module as a reference.
>> 
>> Regards,
>> Niclas Adlertz
>> 
>> (Note that the webrev title is named JDK-7022320. That's just the local folder name that the code resides in. The bug is indeed 8011621)
>> 
>> -----------------------------------------------------------------
>> webrev: http://cr.openjdk.java.net/~adlertz/JDK-8011621/webrev00/
>>    jbs: https://jbs.oracle.com/bugs/browse/JDK-8011621
>>   bugs: http://bugs.sun.com/view_bug.do?bug_id=8011621
>> 
>>