RFR: 8199946: Move load/store and encode/decode out of oopDesc

[Thomas Schatzl]

Hi,

  looks good with minor caveats:

There is another at
psScavenge.inline.hpp
 111       RawAccess<>::oop_store(p, new_obj);

and
metaspaceShared.cpp
1940       RootAccess<IN_ARCHIVE_ROOT>::oop_store(p,
CompressedOops::decode(o));

has been a decode_heap_oop_not_null()

------

Depending on whether you think this fits this change, it would be nice
to fix some code in the following locations. Otherwise I can do a
separate change.

g1OopClosures.inline.hpp:
 244     RawAccess<>::oop_store(p, forwardee);

This could be a RawAccess<OOP_NOT_NULL>::oop_store()

g1ParScanThreadState.inline.hpp:
  49     RawAccess<>::oop_store(p, obj);

Same here.

Thanks,
  Thomas



On Fri, 2018-03-23 at 13:27 +0100, Stefan Karlsson wrote:
> On 2018-03-23 13:28, Erik Österlund wrote:
> > Hi Stefan,
> > 
> > Looks good. There is one remaining OOP_NOT_NULL in 
> > psScavenge.inline.hpp, but I do not need another webrev.
> 
> 
> Fixed. Thanks for the review!
> 
> StefanK
> 
> > 
> > Thanks,
> > /Erik
> > 
> > On 2018-03-22 20:28, Stefan Karlsson wrote:
> > > Erik found two places where I didn't use OOP_NOT_NULL, where we 
> > > previously used _not_null functions.
> > > 
> > > diff --git a/src/hotspot/share/gc/cms/parNewGeneration.cpp 
> > > b/src/hotspot/share/gc/cms/parNewGeneration.cpp
> > > --- a/src/hotspot/share/gc/cms/parNewGeneration.cpp
> > > +++ b/src/hotspot/share/gc/cms/parNewGeneration.cpp
> > > @@ -734,7 +734,7 @@
> > >        oop new_obj = obj->is_forwarded()
> > >                        ? obj->forwardee()
> > >                        : 
> > > _g->DefNewGeneration::copy_to_survivor_space(obj);
> > > -      RawAccess<>::oop_store(p, new_obj);
> > > +      RawAccess<OOP_NOT_NULL>::oop_store(p, new_obj);
> > >      }
> > >      if (_gc_barrier) {
> > >        // If p points to a younger generation, mark the card.
> > > diff --git a/src/hotspot/share/gc/cms/parOopClosures.inline.hpp 
> > > b/src/hotspot/share/gc/cms/parOopClosures.inline.hpp
> > > --- a/src/hotspot/share/gc/cms/parOopClosures.inline.hpp
> > > +++ b/src/hotspot/share/gc/cms/parOopClosures.inline.hpp
> > > @@ -52,7 +52,7 @@
> > >        new_obj = 
> > > ((ParNewGeneration*)_g)->copy_to_survivor_space(_par_scan_state,
> > > obj, obj_sz, m);
> > >      }
> > > -    RawAccess<>::oop_store(p, new_obj);
> > > +    RawAccess<OOP_NOT_NULL>::oop_store(p, new_obj);
> > >    }
> > >  }
> > > 
> > > StefanK
> > > 
> > > On 2018-03-22 17:01, Stefan Karlsson wrote:
> > > > Hi,
> > > > 
> > > > This patch needs Erik's change to the LoadProxies:
> > > > http://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2018-Marc
> > > > h/021504.html 
> > > > 
> > > > 
> > > > to build on fastdebug.
> > > > 
> > > > Here's a rebased patch:
> > > > http://cr.openjdk.java.net/~stefank/8199946/webrev.02/
> > > > 
> > > > Thanks,
> > > > StefanK
> > > > 
> > > > On 2018-03-21 18:27, Stefan Karlsson wrote:
> > > > > Hi all,
> > > > > 
> > > > > Please review this patch to get rid of the
> > > > > oopDesc::load/store 
> > > > > functions and to move the oopDesc::encode/decode functions to
> > > > > a new 
> > > > > CompressedOops subsystem.
> > > > > 
> > > > > http://cr.openjdk.java.net/~stefank/8199946/webrev.01
> > > > > https://bugs.openjdk.java.net/browse/JDK-8199946
> > > > > 
> > > > > When the Access API was introduced many of the usages of 
> > > > > oopDesc::load_decode_heap_oop, and friends, were replaced by
> > > > > calls 
> > > > > to the Access API. However, there are still some usages of
> > > > > these 
> > > > > functions, most notably in the GC code.
> > > > > 
> > > > > This patch is two-fold:
> > > > > 
> > > > > 1) It replaces the oopDesc load and store calls with
> > > > > RawAccess 
> > > > > equivalents.
> > > > > 
> > > > > 2) It moves the oopDesc encode and decode functions to a
> > > > > new, 
> > > > > separate, subsystem called CompressedOops. A future patch
> > > > > could even 
> > > > > move all the Universe::_narrow_oop variables over to
> > > > > CompressedOops.
> > > > > 
> > > > > The second part has the nice property that it breaks up a
> > > > > circular 
> > > > > dependency between oop.inline.hpp and access.inline.hpp.
> > > > > After the 
> > > > > change we have:
> > > > > 
> > > > > oop.inline.hpp includes:
> > > > >    access.inline.hpp
> > > > >    compressedOops.inline.hpp
> > > > > 
> > > > > access.inline.hpp includes:
> > > > >    compressedOops.inline.hpp
> > > > > 
> > > > > Thanks,
> > > > > StefanK
> > > 
> > >