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

Thu Apr 10 19:47:13 UTC 2014

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

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.openjdk.java.net/pipermail/hotspot-compiler-dev/attachments/20140410/ccd2c61e/attachment-0001.html>