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

[Christian Thalinger]

The changes would be much bigger, yes.  Just a thought, I don’t insist on doing it.

> On Apr 30, 2015, at 9:39 AM, Vladimir Kozlov <vladimir.kozlov at oracle.com> wrote:
> 
> We still use OopMap and OopMapSet when we create oopmap data. ImmutableOopMap* is used for writing and reading final metadata. Otherwise changes would be much larger (see for example, sharedRuntime_<arch>.cpp etc). Rickard may correct me if I am wrong.
> 
> Vladimir
> 
> On 4/30/15 9:33 AM, Christian Thalinger wrote:
>> One thing I mentioned to Rickard privately is that I would prefer to have the current OopMap be renamed to something else (TempOopMap?) and keep OopMap as the one used.
>> 
>>> On Apr 30, 2015, at 9:26 AM, Vladimir Kozlov <vladimir.kozlov at oracle.com> 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
>>>>>>>> 
>>