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

[Rickard Bäckman]

On 04/29, Bertrand Delsart wrote:
> On 29/04/2015 16:53, Rickard Bäckman wrote:
> >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.
> 
> Thanks.
> 
> I suppose that ImmutableOopMap.java and ImmutableOopMapSet.java are
> renamings of the old non Immutable code. Did you use 'hg move' ?
> This may help get cleaner history and webrevs, showing what you
> modified in these files.

Thanks, no my idea moved the files for me. Probably not using hg move.
I'll see what I can do about it.

> 
> Note also the missing copyright header in ImmutableOopMapPair.java
> 

Thanks.

/R

> Regards,
> 
> Bertrand.
> 
> >
> >>
> >>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
> >
> 
> 
> -- 
> 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/20150430/a2bf2c16/signature.asc>