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

[David Holmes]

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