RFR (S): 8218458: [TESTBUG] runtime/NMT/CheckForProperDetailStackTrace.java fails with Expected stack trace missing from output

Thomas Stüfe thomas.stuefe at gmail.com
Thu Apr 11 07:33:12 UTC 2019


Hi David,

its okay. If the intent of this test is to check that C-Heap allocating
callstacks are printed in a clear manner with enough significant frames to
identify the caller vs just to test that NMT detail printing in general
works, the test is needed in its complexity.

I still think these are two separate issues but its fine to test them in
one go.

Cheers, Thomas

On Mon, Apr 8, 2019 at 1:40 AM David Holmes <david.holmes at oracle.com> wrote:

> Hi Thomas,
>
> My apologies, I did not mean to ignore your input here. Thanks for
> taking a look and pointing out the scanning error in my original proposal.
>
> Hopefully you are okay with the simpler approach that Chris has advocated.
>
> Thanks,
> David
>
> On 4/04/2019 5:12 pm, Thomas Stüfe wrote:
> > Hi David, Chris,
> >
> > I think this is an improvement and goes in the right direction. Those
> > hard-wired inline guesses always made me twitch a bit.
> >
> > The patch looks fine to me in its current form, since it is already an
> > improvement. So the following remarks are "optional":
> >
> > - Since all we want to do is to test that NMT detail printing works, we
> > do not have to use one of the malloc paths; I have the feeling the mmap
> > paths are more "inline stable" since they usually end up in one of the
> > ReservedSpace child class constructors which do not get inlined.
> >
> > Like this:
> >
> >    74 [0x0000000706400000 - 0x0000000800000000] reserved 4091904KB for
> > Java Heap from
> >    75     [0x00007f9b514cff07]
> > ReservedHeapSpace::try_reserve_range(char*, char*, unsigned long, char*,
> > char*, unsigned long, unsigned long, bool)+0xb7
> >    76     [0x00007f9b514d08d8]
> > ReservedHeapSpace::initialize_compressed_heap(unsigned long, unsigned
> > long, bool)+0x5f8
> >    77     [0x00007f9b514d0f3a]
> > ReservedHeapSpace::ReservedHeapSpace(unsigned long, unsigned long, bool,
> > char const*) [clone .part.29]+0x9a
> >    78     [0x00007f9b51450331] Universe::reserve_heap(unsigned long,
> > unsigned long)+0xe1
> >
> > Or this:
> >
> >   256 [0x00007f9b308c5000 - 0x00007f9b3f8c5000] reserved 245760KB for
> > Code from
> >   257     [0x00007f9b514cad02]
> > ReservedCodeSpace::ReservedCodeSpace(unsigned long, unsigned long,
> > bool)+0xa2
> >   258     [0x00007f9b505bcfb7] CodeCache::reserve_heap_memory(unsigned
> > long)+0xe7
> >   259     [0x00007f9b505bd75b] CodeCache::initialize_heaps()+0x2db
> >   260     [0x00007f9b505bde45] CodeCache::initialize()+0x1b5
> >
> > will be stacks you always will see.
> >
> > - I do not like scanning the whole output for each single stack frame.
> > The test may give false positives. I would like it more if we were to
> > read the file line by line, and when the first pattern line matches,
> > check that subsequent lines match too. This is how we do call stack
> > matching at SAP for similar tests.
> > This is also more efficient since you do not re-scan the whole output
> > each time.
> >
> > In general:
> >
> > NMT is really very useful. We could think about
> > increasing NMT_TrackingStackDepth, since 4 is obviously not a lot. 6 or
> > 8 would be better. I do not believe the memory footprint increase would
> > be significant, but of course we would have to measure.
> >
> > Thanks! Thomas
> >
> > On Thu, Apr 4, 2019 at 8:36 AM Chris Plummer <chris.plummer at oracle.com
> > <mailto:chris.plummer at oracle.com>> wrote:
> >
> >     On 4/3/19 11:23 PM, David Holmes wrote:
> >      > Hi Chris,
> >      >
> >      > On 4/04/2019 4:12 pm, Chris Plummer wrote:
> >      >> Hi David,
> >      >>
> >      >> I have concerns that this will hide some of the other bugs I've
> >      >> mentioned: JDK-8133749, JDK-8133747, and JDK-8133740. These bugs
> >      >> result in 1 or two frames appearing in the stacktrace that
> >     should be
> >      >> skipped. Notably NativeCallStack::NativeCallStack() and
> >      >> os::get_native_stack().
> >      >
> >      > The test still checks those are not present first:
> >      >
> >      > 73         // We should never see either of these frames because
> >     they
> >      > are supposed to be skipped. */
> >      > 74 output.shouldNotContain("NativeCallStack::NativeCallStack");
> >      > 75         output.shouldNotContain("os::get_native_stack");
> >     Ah yes. I skimmed over the test looking for it but missed it.
> >      >
> >      >> Also, AllocateHeap() should normally not be in the stack trace,
> but
> >      >> the test has specifically allowed for it for windows and solaris
> >      >> slowdebug builds. Although these builds should have honored the
> >      >> ALWAYSINLINE directive, it was deemed acceptable that it was not
> in
> >      >> slowdebug builds. However, I would not want to allow
> AllocateHeap()
> >      >> to appear in a product build, and best not to see it in fastdebug
> >      >> either.
> >      >
> >      > This is a test of NMT detail not a test of whether a given
> compiler
> >      > chooses to inline something like AllocateHeap. I don't think it
> >     is the
> >      > job of this test to be checking for something specific to the
> native
> >      > compiler. The previous handling of AllocateHeap seemed to be there
> >      > simply because it was the only way to deal with an optional frame
> -
> >      > but now that's handled generically.
> >     It's appearance means you effectively only have 3 frames to identity
> >     callsites instead of 4. If it does appear in a product build, a
> >     solution
> >     should be looked into to get rid of it. If the port owner decides it
> >     can't get rid of it (or is unwilling to), then an exception should be
> >     added to the test like was done for solaris and windows slowdebug
> >     builds.
> >
> >     thanks,
> >
> >     Chris
> >      >
> >      > Thanks,
> >      > David
> >      >
> >      >> Given the changes you made to allow more flexibly in which frames
> >      >> appear, I think you need to now also make sure the above 3
> >     mentioned
> >      >> frames are not present, except for allowing AllocateHeap() in
> >      >> slowdebug builds.
> >      >>
> >      >> thanks,
> >      >>
> >      >> Chris
> >      >>
> >      >> On 4/3/19 10:53 PM, David Holmes wrote:
> >      >>> Bug: https://bugs.openjdk.java.net/browse/JDK-8218458
> >      >>> Webrev: http://cr.openjdk.java.net/~dholmes/8218458/webrev/
> >      >>>
> >      >>> The actual stack trace reported by NMT detail is affected by the
> >      >>> inlining decisions of the native compiler, and on the type of
> >     build.
> >      >>> So we define an "ideal" stacktrace and then allow for some
> >     frames to
> >      >>> be missing based on empirical observations. So to date we have
> >     seen
> >      >>> two frames that may or may not be inlined and so we allow for 2
> >      >>> non-matching entries.
> >      >>>
> >      >>> The special-casing of AllocateHeap is removed as now it is just
> an
> >      >>> optional frame.
> >      >>>
> >      >>> Chris: does this maintain the "spirit" of the test as you
> intended?
> >      >>>
> >      >>> Zhengyu: can you test this on your system(s) please.
> >      >>>
> >      >>> Thanks,
> >      >>> David
> >      >>
> >      >>
> >
> >
>


More information about the hotspot-runtime-dev mailing list