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

[Igor Ignatyev]

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.

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

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.