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

[Igor Ignatyev]

Vladimir,

>> http://cr.openjdk.java.net/~iignatyev/fzhinkin/rtm/8039499/webrev.01/
> You are passing "-server" flag in prepareFilteredTestOptions(). Where
> you check that only server VM should be tested?
unfortunately, I don't have any comments on it. let's wait for Filipp's 
answer.

>  Also what about -d64/-d32. It may be not problem now since we don't support 32-bit on
> Solaris anymore.
RTMTestBase::executeRTMTest() uses 
ProcessTools::createJavaProcessBuilder() which add '-d64' for Solaris 
64-bit.

> Igor asked for c.o.j.t.compiler.rtm package but for me just rtm is fine
> too.
I'm also fine w/ rtm.

> 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
Sounds reasonable.

-- II

On 04/10/2014 10:43 PM, 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? Also what about
> -d64/-d32. It may be not problem now since we don't support 32-bit on
> Solaris anymore.
>
> 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
>
> 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.
>>