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

Thu Apr 10 19:04:39 UTC 2014

On 4/10/14 12:02 PM, Filipp Zhinkin wrote:
> Vladimir,
>
> On 10.04.2014 22:43, Vladimir Kozlov wrote:
>>
>>
>> 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?
> All RTM tests extends CommandLineOptionTest which takes
> BooleanSupplier's instance as it's constructor arguments.
> That BooleanSupplier's instance used to check if test should be executed
> or skipped.
>
> All non-CLI RTM test pass BooleanSupplier that checks that CPU on test
> box supports RTM feature
> and that current VM is server and not embedded.
>
> In case of any other CPU/VM combination test execution will be skipped.
>
>> Also what about -d64/-d32. It may be not problem now since we don't
>> support 32-bit on Solaris anymore.
> All non-CLI tests take all options passed to JTreg via test.vm.opts and
> test.java.opts,
> filter out all options that can negatively affects test execution (all
> RTM options) and
> adds all options supplied by the test.
>
> So -d32/-d64 options will be passed to test VM.

Thank you for explanation. These changes are good then.

Thanks,
Vlaidmir

>>
>> 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
> OK, I'll change it.
>
> Thanks,
> Filipp.
>>
>> 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.
>>>
>