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

[Vladimir Kozlov]

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.