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

Thu Apr 4 07:12:24 UTC 2019

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>
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
> >>
> >>
>
>
>