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

Thu Apr 10 20:23:05 UTC 2014

Good.

Thanks,
Vladimir

On 4/10/14 12:47 PM, Filipp Zhinkin wrote:
> All CLI test were moved from 'arguments/' to 'cli/' and
> renamed so now their names contain 'Option' instead of 'CLI':
>
> http://cr.openjdk.java.net/~iignatyev/fzhinkin/rtm/8039496/webrev.01/
>
> Thanks,
> Filipp.
>
> On 10.04.2014 23:04, Vladimir Kozlov wrote:
>> 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.
>>>>>
>>>
>