[11] RFR(S): 8207067: [test] prevent timeouts in serviceability/tmtools/jstat/{GcTest02, GcCauseTest02}.java

[Volker Simonis]

On Thu, Jul 12, 2018 at 1:27 AM, David Holmes <david.holmes at oracle.com> wrote:
> Hi Volker,
>
> On 12/07/2018 3:30 AM, Volker Simonis wrote:
>>
>> Hi,
>> can I please have a review for the following test fix which prevents
>> eventual test timeouts:
>>
>> https://bugs.openjdk.java.net/browse/JDK-8207067
>> http://cr.openjdk.java.net/~simonis/webrevs/2018/8207067/
>>
>> The two tests
>> hotspot/jtreg/serviceability/tmtools/jstat/{GcTest02,GcCauseTest02}.java
>> produce more than 90_000 classes until they eat up ~70% of the 128M
>> meta space they run with. The loading of each of these classes
>> triggers a full dependency check for ALL nmethods in debug/fastdebug
>> builds because 'VerifyDependencies' is 'true' there. This slows down
>> the tests from about 3 sec. in the opt build to about 88 sec. in the
>> fastdebug build on x86_64 and from about 4 sec. to about 560 sec. on
>> ppc64.
>>
>> Because the tests are not about dependency checking, it makes sense to
>> switch of 'VerifyDependencies' if they are run inside a
>> debug/fastdebug VM and decrease the execution time down to about 6
>> sec. on both x86_64 and ppc64.
>

Hi David,

thanks for looking at my change!

>
> It's very annoying that this can't be fixed the obvious way by just
> specifying -XX:-VerifyDependencies. Maybe jtreg could add a debug_only(...)
> capability ... :(
>

Agree...

> It's unclear to me whether it is safe to disable VerifyDependencies after
> the VM has commenced execution. This might lead to inconsistencies. Probably
> need the compiler folk to clarify that.
>

I've checked that prior to doing this change. ' VerifyDependencies' is
a very simple flag which maintains no state at all. It is only used
once in codeCache.cpp in 'CodeCache::mark_for_deoptimization()' to
protect the call to 'nmethod::check_all_dependencies(changes)' (i.e.
the one which consumes all the time in the mentioned tests).

Besides that, there are a few uses of ' VerifyDependencies' in
dependencies.cpp which all simply guard some verification code.
Turning ' VerifyDependencies' off in the middle of a compilation or
class loading event will simply prevent further checks without doing
any harm.

But I've CC-ed hotspot-compiler-dev for any case.

> That aside the comments are far too elaborate and better suited for the bug
> report. The tests could use @bug lines (though it would be nice to track
> down the original bug that added the tests). The comments could reduce to a
> simple:
>
> // This test produces more than 90_000 classes until it eats up ~70% of the
> 128M meta space.
> // With VerifyDependencies enabled in debug builds this slows the test down
> considerably.
> // As it is a develop flag, if we see that it is "constant" then we know
> this is a product build.
>

I'm happy to change the comments as suggested by you.

Thank you and best regards,
Volker

> Though there is already Platform.isDebugBuild() if you wanted something more
> direct. (Given you need WB to change the flag it doesn't really make much
> difference.)
>
> Thanks,
> David
>
>
>> Thank you and best regards,
>> Volker
>>
>