RFR (XS) JDK-8153582 Logging of ConcGCThreads is done too early

[Joseph Provino]

Latest webrev with Dima's suggestions:

http://cr.openjdk.java.net/~jprovino/8153582/webrev.02

joe

On 5/18/2016 7:33 AM, Dmitry Fazunenko wrote:
> Hi Joe,
>
> > @requires vm.gc=="ConcMarkSweep" | vm.gc=="G1" | vm.gc=="null"
>
> Will not help unfortunately :(
>
> This test will fail on minimal VM where serial is always selected
> You need:
>    @requires vm.flavor != "minimal"
>
>
> The way you spawn a new VM ignores external flags:
>
>     pb = ProcessTools.createJavaProcessBuilder("-XX:+" + gcFlag, "-Xlog:gc=debug", "-version");
>
> It's recommended to NOT ignore external flags:
>     pb = ProcessTools.createJavaProcessBuilder(*true, *"-XX:+" + gcFlag, "-Xlog:gc=debug", "-version");
>
> But in this case the test will not work if either G1 or CMS is set explicitly:
>    verifyNumberOfConcurrentAndParallelGCThreads(null);
>    verifyNumberOfConcurrentAndParallelGCThreads("UseG1GC");
>    verifyNumberOfConcurrentAndParallelGCThreads("UseConcMarkSweepGC");
> either second or third case will fail because of collector conflict.
> To avoid collector conflicts you need:
>    @requires vm.gc=="null"
> But in this case,
>    verifyNumberOfConcurrentAndParallelGCThreads(null);
> will fail if ergonomics will select Serial or Parallel...
>
>
> What I suggest
>
> add:
>    @requires vm.flavor != "minimal"
>    @requires vm.gc=="null"
>
> get rid of the null case
>    40         verifyNumberOfConcurrentAndParallelGCThreads(null);
>    41
>    42         verifyNumberOfConcurrentAndParallelGCThreads("UseG1GC");
>    43
>    44         verifyNumberOfConcurrentAndParallelGCThreads("UseConcMarkSweepGC");
>
> ->
>               verifyNumberOfConcurrentAndParallelGCThreads("UseG1GC");
>               verifyNumberOfConcurrentAndParallelGCThreads("UseConcMarkSweepGC");
>
>
> don't ignore external flags:
>    50         if (gcFlag != null) {
>    51             pb = ProcessTools.createJavaProcessBuilder("-XX:+" + gcFlag, "-Xlog:gc=debug", "-version");
>    52         } else {
>    53             pb = ProcessTools.createJavaProcessBuilder("-Xlog:gc=debug", "-version");
>    54         }
> -->
>               pb = ProcessTools.createJavaProcessBuilder(*true*, "-XX:+" + gcFlag, "-Xlog:gc=debug", "-version");
>   
> Thanks,
> Dima
>
>
> On 18.05.2016 11:12, Thomas Schatzl wrote:
>> Hi again,
>>
>> On Wed, 2016-05-18 at 10:09 +0200, Thomas Schatzl wrote:
>>> Hi Joe,
>>>
>>> On Tue, 2016-05-17 at 15:17 -0400, Joseph Provino wrote:
>>>> I changed the log level to debug and added created a test.
>>>>
>>>> Webrev:http://cr.openjdk.java.net/~jprovino/8153582/webrev.01
>>>>
>>>    in the test, please limit the allowable GCs to CMS and G1. I do not
>>> think it makes sense to call the test with other GCs.
>>>
>>> Like
>>>
>>> @requires vm.gc=="ConcMarkSweep" | vm.gc=="G1" | vm.gc=="null"
>> Also the copyright date should be 2016 only.
>>
>>> I do not need a re-review for this change.
>> Still applies.
>>
>> Thanks,
>>    Thomas
>