RFR(XXS) : 8177709 : Convert TestVirtualSpace_test to GTest

[JC Beyler]

Hi Igor,

Looks good to me as well, I agree with Thomas' comments and also prefer
RAII, especially since SetUp/TearDown here is going to be weird since you
don't always create the object the same way.

If you make a good RAII object, then both tests could use the same kind of
code instead of one using a helper method and another creating the
ReservedSpace directly too.

Thanks!
Jc

On Wed, Oct 17, 2018 at 11:54 AM Thomas Stüfe <thomas.stuefe at gmail.com>
wrote:

> On Wed, Oct 17, 2018 at 8:29 PM Igor Ignatyev <igor.ignatyev at oracle.com>
> wrote:
> >
> > Hi Thomas,
> >
> > 1st of all, thanks for looking at it, appreciate that.
> >
> > it seems we can. there are two ways I can fix that either replace ASSERT
> by EXCEPT or introduce a text fixture w/ SetUp and TearDown. which do you
> prefer?
> >
> > -- Igor
>
> Slight preference for SetUp/TearDown (EXCEPT would quit the testing
> altogether, wont it?).
>
> Lazy me would have just used an RAII object like this:
>
> struct Cleaner {
>   ReservedSpace* rs;
>   ~Cleaner() { rs.release(); }
> };
>
> But I leave it up to you.
>
> ... Thomas
>
> >
> > > On Oct 17, 2018, at 11:16 AM, Thomas Stüfe <thomas.stuefe at gmail.com>
> wrote:
> > >
> > > Hi Igor,
> > >
> > > test_virtual_space_actual_committed_space() and
> > > actual_committed_space_one_large_page():
> > >
> > > Won't we leak the reserved space now when we run into an ASSERT and
> > > then continue with the next gtests?
> > >
> > > Cheers, Thomas
> > >
> > > On Wed, Oct 17, 2018 at 7:55 PM Igor Ignatyev <
> igor.ignatyev at oracle.com> wrote:
> > >>
> > >> http://cr.openjdk.java.net/~iignatyev//8177709/webrev.00/index.html
> > >>> 141 lines changed: 140 ins; 1 del; 0 mod;
> > >>
> > >> Hi all,
> > >>
> > >> could you please review this small (and hopefully trivial) patch
> which converts internal TestVirtualSpace_test to gtest?
> > >> again, the old test is still used by WhiteBox::runMemoryUnitTests,
> the old code hasn't been removed. it will be removed later when all 4 tests
> used by runMemoryUnitTests are converted.
> > >>
> > >> for the sake of reviewer,
> http://hg.openjdk.java.net/jdk/jdk/file/2e928420389d/src/hotspot/share/memory/virtualspace.cpp#l1261
> is the old version.
> > >>
> > >> webrev:
> http://cr.openjdk.java.net/~iignatyev//8177709/webrev.00/index.html
> > >> JBS: https://bugs.openjdk.java.net/browse/JDK-8177709
> > >> testing:
> > >> - converted tests on linux-x64, windows-x64, macosx-x64,
> solaris-sparcv9 in product and fastdebug variants
> > >> - build w/ precompiled-headers enabled and disabled
> > >>
> > >> Thanks,
> > >> -- Igor
> >
>


-- 

Thanks,
Jc
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://mail.openjdk.org/pipermail/hotspot-gc-dev/attachments/20181017/4be02993/attachment.htm>