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

Fri Apr 11 07:51:51 UTC 2014

Vladimir, thank you for review.

Filipp.

On 04/11/2014 12:23 AM, Vladimir Kozlov wrote:
> 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.
>>>>>>
>>>>
>>