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

[Filipp Zhinkin]

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/

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

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/

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.

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/attachments/20140410/13d633cc/attachment-0001.html>