RFR (M): 8064458 OopMap class could be more compact
vladimir.kozlov at oracle.com
Wed Apr 29 17:21:48 UTC 2015
On 4/29/15 7:52 AM, Rickard Bäckman wrote:
> On 04/28, Vladimir Kozlov wrote:
>> You need closed SA changes.
> I have them, just forgot to send the mail. Small changes too.
>> Move fields to the beginning of ImmutableOopMapBuilder and classes.
>> Add comments describing each field in ALL new classes. Add comments
>> to fields in old class. It will help next persone who will look on
>> oop maps later.
> All fields are at the beginning? Just following style and keeping
> friends above fields as other classes in the same file has.
It is better to see fields before methods which access them. I am not sure what codding style you are talking about. All
classes in oopMap.hpp has above layout.
I am asking to change layout of ImmutableOopMapBuilder and Mapping classes. Also why Mapping is inner class?
>> Add ResourceMark into ImmutableOopMapSet::build_from() to free memory allocated in ImmutableOopMapBuilder().
no ResourceMark in 8064458.2
>> Why you need ImmutableOopMapBuilder to be friend of class ImmutableOopMap?
> It makes a call to data_addr() which is private. Only for debug builds
> so I wrapped it in DEBUG_ONLY.
Put ImmutableOopMapBuilder::verify() under #ifdef ASSERT and its declaration in DEBUG_ONLY.
>> I think a simple loop in ImmutableOopMapBuilder::verify() would be faster than calling memcmp.
>> Field _end is not used.
>> ImmutableOopMapBuilder() calls reset() and next called heap_size()
>> calls reset() again. May be move reset() to the end of heap_size()
>> so that you don't need to call it in fill().
> Better yet, the reset() method isn't required anymore.
> Updated webrev: http://cr.openjdk.java.net/~rbackman/8064458.2
>> On 4/28/15 3:03 AM, Rickard Bäckman wrote:
>>> Hi all,
>>> can I please have reviews for this change:
>>> RFR: http://cr.openjdk.java.net/~rbackman/8064458.1/
>>> RFE: http://bugs.openjdk.java.net/browse/JDK-8064458
>>> While looking at OopMaps a while ago I noticed that there were a couple
>>> of different fields that were unused after the OopMaps were finalised.
>>> I took some time to investigate and rearrange the OopMaps. Since I
>>> didn't want to change how the OopMaps are built I introduced new data
>>> structures. ImmutableOopMapSet and ImmutableOopMap. The original OopMap
>>> structures are used to build up the OopMaps and when finalised they are
>>> copied into the Immutable variants.
>>> The ImmutableOopMapSet contains a few fields [size, count] and then a
>>> list of [pc, offset]. The offset points to the offset after the list
>>> where the ImmutableOopMap is placed. By moving pc out from OopMap to be
>>> part of the list we can now have multiple pcs with identical OopMaps
>>> point to the same data.
>>> We only keep 1 empty OopMap, and the other compaction that is done in
>>> this change is to check if the OopMap is identical to the previous one
>>> and then reuse that one. So no complete uniqueness check.
>>> I ran a couple of small benchmarks and printed the size of the old
>>> OopMaps vs the new. The new layout uses about 20 - 25% of the space on
>>> the benchmarks I've run.
>>> Tested by running through JPRT, running BigApps and NSK.quick.testlist
More information about the hotspot-compiler-dev