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