RFR (S): 8227671: G1: assert_used_and_recalculate_used_equal performs work in product builds

[Thomas Schatzl]

Hi,

On Mon, 2019-07-15 at 15:03 -0400, Kim Barrett wrote:
> > On Jul 15, 2019, at 11:38 AM, Thomas Schatzl <
> > thomas.schatzl at oracle.com> wrote:
> > 
> > Hi all,
> > 
> >  can I have reviews for this small change that avoids doing
> > unneccessary work in product builds?
> > 
> > In particular, the assert_used_and_recalculate_used_equal macro
> > executes code (the recalculate_used() call) that is potentially
> > lengthy.
> > 
> > I measured a few ms on a large heap (50GB) with (forced) 50k
> > regions. I am not sure why this call is so expensive in my tests,
> > but it is completely unnecessary.
> > 
> > CR:
> > https://bugs.openjdk.java.net/browse/JDK-8227671
> > Webrev:
> > http://cr.openjdk.java.net/~tschatzl/8227671/webrev/
> > Testing:
> > Currently running hs-tier1-3, local gc/g1 jtreg run, local
> > benchmarks
> > 
> > Thanks,
> >  Thomas
> 
> Looks good.
> 
> One minor nit issue, for which I don't need a new webrev.
> 
> -------------------------------------------------------------------
> -----------
> src/hotspot/share/gc/g1/g1CollectedHeap.hpp
>  366 #define assert_used_and_recalculate_used_equal(g1h) while(0)
> 
> That should probably be "do {} while (0)".  It probably doesn't
> matter too much, since the case that can go wrong in a non-debug
> build will probably not compile in a debug build.

Fixed and regenerated webrev.

> 
> -------------------------------------------------------------------
> ----------- 
> src/hotspot/share/gc/g1/g1CollectedHeap.hpp
>  356 #ifdef ASSERT
>  357 #define assert_used_and_recalculate_used_equal(g1h)
> 
> I wonder why this is a macro and not a NOT_DEBUG_RETURN function.
> 
> The same goes for some of the others nearby.  I think macros were
> used because we were getting poor stacktrace info in hs_err files,
> but I thought that was pretty much fixed.
> 
> But this is all a separate issue from the performance bug being
> addressed by this change.

I filed JDK-8227713 for that.

Thanks,
  Thomas