RFR: 8212630: jtreg/gc/logging/TestMetaSpaceLog.java failed with Agent timed out

[Kim Barrett]

> On May 30, 2019, at 6:22 PM, Leonid Mesnik <leonid.mesnik at oracle.com> wrote:
> 
> Hi
> 
> I think that '@run main/othervm -Xbootclasspath/a:. -XX:+UnlockDiagnosticVMOptions -XX:+WhiteBoxAPI' could be replaced with @run main/driver because
> test spawns other process and don't use any options and WB.

The main tests are using sun.hotspot.gc.GC, which uses WB.

> Also I think you need to have several separate headers for each test instead of multiple runs. There is only one result per test (not for every run) and test is is skipped if any of GC is not supported. Additionally these tests might be guarded with require instead of throwing SkipException. So your tests is not going to be executed saying if ZGC is selected by jtreg CLI. Here is the example of fix which update test with different GCs. It contains a lot of examples of similar tests.
> http://hg.openjdk.java.net/jdk/jdk/rev/9b8926bf85c1
> 
> And in the case of new timeouts/other failures we will know exactly which GC was used right in the report.

I thought about using multiple @tests rather than multiple @runs, but
decided not to take that path, as doing so is much more verbose, for
not much (if any) benefit. The verbosity comes from duplicating most
of the @test block N times (including build and install WB support,
since the child process uses it too), rather than just having N @run
lines plus a little bit of code in main to look up the requested GC
object and dispatch on whether it's supported. I don't think getting N
results rather than 1 and avoiding the SkippedException case are
worth that "code" duplication. (The multiple @test use-case really
wants some sort of template mechanism.)

Note that I tested it with an additional @run line for Shenandoah
(with it configured out as usual for our builds, so not supported),
and that worked fine, giving the expected "passed because skipped"
result.

This test wouldn't have been executed with ZGC or Shenandoah
previously either. If we want it to test those we should now add new
@run lines for them. (Or new @test blocks, but that just makes my
verbosity complaint even worse.) (The old version required adding
"if-supported then test" blocks for them.) (And note that this test
fails as expected for Epsilon.)

I see several other tests in that changeset that could be restructured
like I'm doing here (any using GC.isSupported() seem like candidates
for this same treatment), perhaps avoiding similar timeouts for
similar reasons.

> 
> Leonid
> 
>> On May 30, 2019, at 12:29 PM, Kim Barrett <kim.barrett at oracle.com> wrote:
>> 
>> […]
>> CR:
>> https://bugs.openjdk.java.net/browse/JDK-8212630
>> 
>> Webrev:
>> http://cr.openjdk.java.net/~kbarrett/8212630/open.00/
>> 
>> Testing:
>> A few dozen local executions of the test.  That's not enough and
>> probably not in a configuration to encounter the problem -- failures
>> have been occurring on the order of 0.5% of the time under high load.