RFR (XL): Tests on Intel RTM instructions for locks

Thu Apr 10 19:02:26 UTC 2014

Vladimir,

On 10.04.2014 22:43, Vladimir Kozlov wrote:
>
>
> On 4/10/14 11:11 AM, Filipp Zhinkin wrote:
>> Vladimir, Igor,
>>
>> thanks for looking at it!
>>
>> Changes:
>>
>> - all RTM-related stuff was moved from test/testlibrary/ to
>>    test/compiler/testlibrary/rtm, also VM flags common to all tests are
>>    now set up by RTMTestBase::executeRTMTest:
>>
>> http://cr.openjdk.java.net/~iignatyev/fzhinkin/rtm/8039499/webrev.01/
>
> In general it looks good.
> You are passing "-server" flag in prepareFilteredTestOptions(). Where 
> you check that only server VM should be tested? 
All RTM tests extends CommandLineOptionTest which takes 
BooleanSupplier's instance as it's constructor arguments.
That BooleanSupplier's instance used to check if test should be executed 
or skipped.

All non-CLI RTM test pass BooleanSupplier that checks that CPU on test 
box supports RTM feature
and that current VM is server and not embedded.

In case of any other CPU/VM combination test execution will be skipped.

> Also what about -d64/-d32. It may be not problem now since we don't 
> support 32-bit on Solaris anymore.
All non-CLI tests take all options passed to JTreg via test.vm.opts and 
test.java.opts,
filter out all options that can negatively affects test execution (all 
RTM options) and
adds all options supplied by the test.

So -d32/-d64 options will be passed to test VM.
>
> Igor asked for c.o.j.t.compiler.rtm package but for me just rtm is 
> fine too.
>
>>
>> - tests were updated to use common classes from new place and all common
>>    VM flags removed from tests:
>>
>> http://cr.openjdk.java.net/~iignatyev/fzhinkin/rtm/8037860/webrev.01/
>
> Looks fine to me.
>
>>
>> Also, I'd like to put here a webrev for command line options tests 
>> for all
>> RTM-related options. Tests are pretty simple and just verify that all
>> options
>> could be used and options' processing preformed correctly:
>>
>> http://cr.openjdk.java.net/~iignatyev/fzhinkin/rtm/8039496/webrev.00/
>
> Test look fine but I would use 'cli/' instead of 'arguments/' in 
> directory name and use 'Option' instead of 'CLI' in tests name:
>
> test/compiler/rtm/cli/TestRTMLockingThresholdOption.java
OK, I'll change it.

Thanks,
Filipp.
>
> Thanks,
> Vlaidmir
>
>>
>> Thanks,
>> Filipp.
>>
>> On 10.04.2014 11:24, Igor Ignatyev wrote:
>>> Hi Vladimir,
>>>
>>>> But you should separate them in a such way that you can push them
>>>> separately and sequentially based on dependencies.
>>>> If changes depends on each other (both directions), they should be in
>>>> one changeset.
>>> thanks for the explanation, in brief you're also for '1-1' rule (if
>>> it's possible).
>>>
>>>> As I understand current changes could be pushed separately in the 
>>>> order
>>>> they are listed. So there is no problem here. Right?
>>> yes.
>>>
>>> On 04/10/2014 11:13 AM, Vladimir Kozlov wrote:
>>>> Hi Igor,
>>>>
>>>> On 4/9/14 11:41 PM, Igor Ignatyev wrote:
>>>>> Vladimir,
>>>>>
>>>>>> RTM is compiler's feature. I think you should not put it into 
>>>>>> general
>>>>>> testlibrary directory. Is it possible to have RTM extension of
>>>>>> testlibrary in compiler directory?
>>>>> yes, it's possible. we can create 'test/compiler/test/testlibrary' 
>>>>> and
>>>>> put all our stuff there. then for rtm classes I'd
>>>>> prefer to have c.o.j.t.compiler.rtm package.
>>>>
>>>> Good.
>>>>
>>>>>
>>>>>>> Should all these changes pushed at once or one by one?
>>>>> what do you think about it? common testlibrary classes definitely
>>>>> should be pushed separately, but what about other?
>>>>> tests from 8037860 as well as from 4th part depend on testlibrary
>>>>> changes, and they aren't applicable w/o them.
>>>>>
>>>>> I'm asking because gc-people want to follow the rule '1 changeset 
>>>>> -- 1
>>>>> CR', even for very small fixes, even if one
>>>>> depends on another. in my opinion, it's unjustified in many cases. //
>>>>> Not sure about our current one
>>>>
>>>> It is better if changes could be separated into smaller parts which 
>>>> are
>>>> easier to review and test (search for a failure cause).
>>>>
>>>> But you should separate them in a such way that you can push them
>>>> separately and sequentially based on dependencies.
>>>> If changes depends on each other (both directions), they should be in
>>>> one changeset.
>>>> As I understand current changes could be pushed separately in the 
>>>> order
>>>> they are listed. So there is no problem here. Right?
>>>>
>>>> Thanks,
>>>> Vladimir
>>>>
>>>>>
>>>>> Igor
>>>>>
>>>>> On 04/10/2014 02:25 AM, Vladimir Kozlov wrote:
>>>>>> On 4/9/14 2:08 PM, Filipp Zhinkin wrote:
>>>>>>> Hi all,
>>>>>>>
>>>>>>> I'd like to add test on Intel's RTM support.
>>>>>>> There is to much code to review at once, so I've split it into 4
>>>>>>> parts.
>>>>>>> This review contains first three parts:
>>>>>>> - changes in common testlibrary classes ( 8039497)
>>>>>>> http://cr.openjdk.java.net/~iignatyev/fzhinkin/rtm/8039497/webrev.00/ 
>>>>>>>
>>>>>>> <http://cr.openjdk.java.net/%7Eiignatyev/fzhinkin/rtm/8039497/webrev.00/> 
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>>> This look good.
>>>>>>
>>>>>>> - new testlibrary classes used by tests ( 8039499)
>>>>>>> http://cr.openjdk.java.net/~iignatyev/fzhinkin/rtm/8039499/webrev.00/ 
>>>>>>>
>>>>>>> <http://cr.openjdk.java.net/%7Eiignatyev/fzhinkin/rtm/8039499/webrev.00/> 
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>>> Some new files are missing Copyright header.
>>>>>> RTM is compiler's feature. I think you should not put it into 
>>>>>> general
>>>>>> testlibrary directory. Is it possible to have RTM extension of
>>>>>> testlibrary in compiler directory?
>>>>>> The code looks fine.
>>>>>>
>>>>>>> - tests on different RTM options and their combinations (8037860)
>>>>>>> http://cr.openjdk.java.net/~iignatyev/fzhinkin/rtm/8037860/webrev.00/ 
>>>>>>>
>>>>>>> <http://cr.openjdk.java.net/%7Eiignatyev/fzhinkin/rtm/8037860/webrev.00/> 
>>>>>>>
>>>>>>>
>>>>>>>
>>>>>>
>>>>>> The new code is huge and I only glanced through it :)
>>>>>> The only thing jumped on me is the repeated list of passed flags to
>>>>>> executeRTMTest() in each test. Is it possible to consolidate common
>>>>>> flags setting in one place?
>>>>>>
>>>>>> Thanks,
>>>>>> Vladimir
>>>>>>
>>>>>>>
>>>>>>> The fourth part which was not included into this review request is
>>>>>>> sanity tests on command line options processing.
>>>>>>>
>>>>>>> Detailed description of changes:
>>>>>>> - changes in common testlibrary classes (8039497):
>>>>>>>    a) c.o.j.t.Platform was updated to provide information abort 
>>>>>>> more
>>>>>>>        different types of VM.
>>>>>>>    b) c.o.j.t.Utils was updated to provide access to Unsafe
>>>>>>>
>>>>>>> - new testlibrary classes used by tests (8039499):
>>>>>>>    a) All classes used by new tests on RTM were added to
>>>>>>>        com.oracle.java.testlibrary.rtm package.
>>>>>>>        Such classes include different RTM's abort provokers,
>>>>>>>        RTMLockingStatistics parsers and other utility classes.
>>>>>>>
>>>>>>>    b) c.o.j.t.cli.* classes were updated to reduce complexity of
>>>>>>> tests.
>>>>>>>
>>>>>>> - tests on different RTM options and their combinations (8037860):
>>>>>>>    Tests on all new RTM-related command line options and
>>>>>>>    new method's options were developed. Tests verify correctness
>>>>>>>    of RTM locking depending on different options values and their
>>>>>>>    combinations.
>>>>>>>
>>>>>>> Should all these changes pushed at once or one by one?
>>>>>>>
>>>>>>> Thanks,
>>>>>>> Filipp.
>>