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

[Vladimir Kozlov]

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