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

[Bertrand Delsart]

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.

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{"

- 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.

- 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. ]

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.
~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~