[aarch64-port-dev ] RFR: 8205694: AArch64: Add test to validate volatile load, store and CAS code generation

[Dmitrij Pochepko]

On 27.06.2018 13:54, Andrew Dinn wrote:
> Hi Dmitrij,
>
> On 26/06/18 19:33, Dmitrij Pochepko wrote:
>> Overall looks good, but I have few minor comments regarding these tests:
> Thanks for reviewing the patch.
I'm not an official reviewed though
>> 1. Most tests which spawn other precesses for testing purposes are
>> launched as "@run driver ..." for optimization reasons.
> I'm using the Process API so I can retrieve and analyze the output
> written to the spawned JVM's stdout. Is it possible to do that using
> "@run driver"?
yes
>> 2. Do we really need to ignore external vm options set in jtreg via
>> option: -javaoptions:"<additional options>"? In case these tests
>> shouldn't ignore external options, there is
>> ProcessTools.createJavaProcessBuilder(boolean addTestVMAndJavaOptions,
>> String... command) for that.
> Hmm, good question. I cannot see a need to pass on any extra test
> options to the spawned JVMs at the moment. The only reason to create the
> spawned process is to observe how the JIT generates code for volatile
> ops and, as far as I am aware, all relevant JIT and GC options that
> affect that aspect of its operation are specified in the test. However,
> I may well have overlooked something.
>
> Do you have a specific reason for thinking that certain JVM options need
> to be passed on? If not perhaps we could consider passing on extra
> javaoptions if/when a case for it arises.
Well, not particular reason for that. There are few examples thought, 
like launching testing with some agent, or specific heap size e.t.c.
>
>> 3. This test will fail if jdk will be built without all GCs or without
>> C2. It probably a good idea to split it in few tests with separate GC
>> configurations
> Well, I had thought about splitting out separate tests for some GCs,
> although I was only motivated to do so for Shenandoah and ZGC -- to cope
> with their potential presence/absence.
>
> The test may also eventually need to allow for future disappearance of
> CMS (currently deprecated). However, that event will also require
> changes to the JIT code to make the case handling for CMS conditional on
> its presence/absence -- I am inclined to postpone any such surgery on
> the current test until CMS goes away.
>
> However, I don't understand how splitting the tests up is going to help
> deal with the either of the two config omissions you are concerned
> about.
It is a test granularity question. Splitting tests will reduce time for 
failure diagnostic. When looking at test report it makes a difference at 
first glance to see: TestVolatiles.java failed or TestVolatilesG1.java 
failed, or all TestVolatiles*.java failed. If you think this granularity 
is fine, then let it be.
>   Consider first the question of whether C2 is present. I could
> specify @requires vm.flavor=="server" in the header comment but I
> believe that would only indicate that "--server" has been passed as an
> argument to the jtreg test jvm. That flag could be absent and the JVM
> could still include a C2 compiler. However, by adding it as a
> requirement the result would be that the test would only get run if the
> test run explicitly passes "-server" i.e. it would effectively disable
> the test.
>
> Similarly, if I put, say, the parallel tests into a separate file I
> could then add @requires vm.gc = "Parallel". However, once again I
> believe that would only detect whether the jtreg test jvm had been
> started using -XX:+UseParallelGC. It would not ensure that the test
> would be run using a Parallel GC when some other GC was configured for
> jtreg. Nor would it identify whether that a Parallel GC was available
> into the JVM being tested. SO, once again it would disable all tests
> (except, perhaps, those for G1).
>
> If I have misunderstood how the @requires config works and how splitting
> the tests up would improve clarity of test outcomes then I'd be happy to
> be corrected here.
It's not like that. vm.flavor doesn't check flags. It parse vm property 
"java.vm.name".
vm.gc.* are also not that simple. You can check how additional 
"requires" properties are constructed in 
test/jtreg-ext/requires/VMProps.java

However, since you probably won't pass external options, you won't need 
it. Just FYI.
>
> Also, I am not really sure how important it is to make this test work
> when the config omits C2 or removes all GCs. This test is needed to make
> sure that the standard AArch64 build which includes C2 and all GCs is
> not broken. That's the critical case to validate. I'd prefer to leave
> this as is for now unless and until jtreg makes it possible to identify
> whether a specific JIT or GC is available in a JVM under test as opposed
> to testing what GC is configured on the jtreg command line.
ok
>
>> 4. In case external options are not ignored, you'll need to filter out
>> several vm options, like skipping execution for c1 or interpreter
>> testruns. Something like this can be added: @requires vm.compMode !=
>> "Xint" & vm.flavor == "server" & (vm.opt.TieredStopAtLevel == null |
>> vm.opt.TieredStopAtLevel==4)
> I don't think that is needed given that I don't intend to pass on the
> jtreg test options.
ok
>
> regards,
>
>
> Andrew Dinn
> -----------
> Senior Principal Software Engineer
> Red Hat UK Ltd
> Registered in England and Wales under Company Registration No. 03798903
> Directors: Michael Cunningham, Michael ("Mike") O'Neill, Eric Shander