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

Vladimir Kozlov vladimir.kozlov at oracle.com
Mon May 4 18:54:58 UTC 2015


Keep it as you have it now in your changes.

thanks,
Vladimir

On 5/4/15 6:16 AM, Rickard Bäckman wrote:
> Vladimir,
>
> I think you are right. Personally I don't see much improvement by having
> TempOopMap/OopMap vs OopMap/ImmutableOopMap (if we had had namespaces we
> could have had them as compiler::OopMap/code::OopMap). If I see lots of requests
> for the former before I push...
>
> Thanks
> /R
>
> On 04/30, Vladimir Kozlov 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
>>>>>>>>>
>>>


More information about the hotspot-compiler-dev mailing list