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

[Rickard Bäckman]

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
> >>>>>>>
> >
-------------- 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/20150504/da53fd23/signature-0001.asc>