RFR (M): 8064458 OopMap class could be more compact

[Vladimir Kozlov]

Looks good.

Thanks,
Vladimir

On 5/4/15 6:13 AM, Rickard Bäckman wrote:
> Updated again.
>
> http://cr.openjdk.java.net/~rbackman/8064458.5
>
> Thanks
>
> On 04/30, Vladimir Kozlov wrote:
>> It is better now. I am fine with inner Mapping class.
>>
>> One thing left. I think only Mapping and fields could be private since only ImmutableOopMapBuilder accesses them:
>>
>>
>> + class ImmutableOopMapBuilder {
>> + public:
>> +   class Mapping;
>> +
>> + public:
>> +   const OopMapSet* _set;
>>
>> thanks,
>> Vladimir
>>
>> On 4/30/15 7:18 AM, Rickard Bäckman wrote:
>>> On 04/29, Vladimir Kozlov wrote:
>>>> 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.
>>>>>
>>>>>>
>>>>>> Style:
>>>>>>
>>>>>> 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.
>>>>
>>>>    friends
>>>>    fields
>>>>    methods
>>>>
>>>> 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?
>>>
>>> Ah, I was looking at oopMap.hpp and didn't see them. Fixed.
>>> Mapping is an inner class because it is only used by the Builder and I
>>> didn't want to clutter the namespace with something as general (and bad)
>>> as Mapping. I can move it out if that is preferred.
>>>
>>>>
>>>>
>>>>>
>>>>>>
>>>>>> Add ResourceMark into ImmutableOopMapSet::build_from() to free memory allocated in ImmutableOopMapBuilder().
>>>>
>>>> no ResourceMark in 8064458.2
>>>
>>> Added.
>>>
>>>>
>>>>>> 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.
>>>>
>>>
>>> Done.
>>>
>>> Updated webrev: http://cr.openjdk.java.net/~rbackman/8064458.4
>>>
>>> Thanks
>>> /R
>>>
>>>> Thanks,
>>>> Vladimir
>>>>
>>>>>
>>>>>>
>>>>>> I think a simple loop in ImmutableOopMapBuilder::verify() would be faster than calling memcmp.
>>>>>
>>>>> Fixed.
>>>>>
>>>>>>
>>>>>> Field _end is not used.
>>>>>
>>>>> Removed.
>>>>>
>>>>>>
>>>>>> 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
>>>>>
>>>>> /R
>>>>>
>>>>>> Thanks,
>>>>>> Vladimir
>>>>>>
>>>>>> 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
>>>>>>>
>>>>>>> Thanks
>>>>>>> /R
>>>>>>>
> /R
>