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

[Igor Ignatyev]

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.