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

[Rickard Bäckman]

On 04/28, Bertrand Delsart wrote:
> Hi,
> 
> First, thanks for the change. The additional benefit is that an
> ImmutableOopMapSet no longer contains any absolute references.
> 
> A few comments.
> 
> The ImmutableOopMapSet.java seems to be missing in the webrev.

Added.

> 
> There also seem to be a few issues with ImmutableOopMap::print_on
> and ImmutableOopMapSet::print_on:
> 
> - I did not spot the closing "}" corresponding to
>   "ImmutableOopMap{"

Fixed.

> 
> - the "map != last" part does not look complete. I assume that you
> are trying to dump only once the OopMap when it is shared by
> successive pcs but you are then missing a "last = map;" line
> somewhere in the if statement.

Thanks, missed that. Fixed.

> 
> - as a minor point, I'd rather print "offs:" or "pc offsets:"
> instead of "pcs:" because OopMapSet use pc offsets, not absolute
> pcs. [ Your CR might also be the right time to replace "pc" by
> "pc_offset" for a few field or variable names since this is a bit
> confusing. ]

Changed to "pc offsets:". Renamed some of the new fields as well. Didn't
want to mess with the old ones.

Updated webrev: http://cr.openjdk.java.net/~rbackman/8064458.2/


Thanks
/R

> 
> Regards,
> 
> Bertrand.
> 
> On 28/04/2015 12:03, 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
> >
> 
> 
> -- 
> Bertrand Delsart,                     Grenoble Engineering Center
> Oracle,         180 av. de l'Europe,          ZIRST de Montbonnot
> 38330 Montbonnot Saint Martin,                             FRANCE
> bertrand.delsart at oracle.com             Phone : +33 4 76 18 81 23
> 
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> NOTICE: This email message is for the sole use of the intended
> recipient(s) and may contain confidential and privileged
> information. Any unauthorized review, use, disclosure or
> distribution is prohibited. If you are not the intended recipient,
> please contact the sender by reply email and destroy all copies of
> the original message.
> ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
/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/1be9982a/signature.asc>