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

[Rickard Bäckman]

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.

> 
> Add ResourceMark into ImmutableOopMapSet::build_from() to free memory allocated in ImmutableOopMapBuilder().
> 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.

> 
> 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
> >
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: Digital signature
URL: <http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/attachments/20150429/b7062db6/signature.asc>