RFR(S) : 8011675: adding compilation level to replay data

Vladimir Kozlov vladimir.kozlov at oracle.com
Wed Apr 17 10:36:01 PDT 2013 

This looks good.

Thanks,
Vladimir

On 4/17/13 10:08 AM, Igor Ignatyev wrote:
> http://cr.openjdk.java.net/~iignatyev/8011675/webrev.05/
>
>> TestVM_no_comp_level.sh:
>>
>>    61     negative_test 10 -XX:-TieredCompilation -client
>>    62     negative_test 11 -XX:+TieredCompilation -client
>>
> replaced by one negative test with just '-client' as option
>
>> common.sh:
>>
>>   107         negative_test $1 -XX:-TieredCompilation -client
>>   108         negative_test `expr $1 + 1` -XX:+TieredCompilation -client
>>
> also replaced by one negative test with just '-client' as option
>
>> Also instead of number for error code can you use messages? It will be
>> very difficult to trace which test failed by number especially since you
>> use expressions for it.
> added test name to test's output. Using of different exit codes is
> necessary for better bug matching.
>
>> We should disable core files generation and tests on Mac OS. The Nightly
>> should be clean.
> 'TestSA.sh' will be skipped on Mac OS, if it isn't configured for core
> dumping
>
> Best regards,
> Igor Ignatyev
>
> On 04/16/2013 09:55 PM, Vladimir Kozlov wrote:
>> Sorry, I notice only now that next tests will fail because there is no
>> Tiered in Client VM and not because there is a problem in Replay code.
>> So it does not test Replay code:
>>
>> TestVM_no_comp_level.sh:
>>
>>    61     negative_test 10 -XX:-TieredCompilation -client
>>    62     negative_test 11 -XX:+TieredCompilation -client
>>
>> common.sh:
>>
>>   107         negative_test $1 -XX:-TieredCompilation -client
>>   108         negative_test `expr $1 + 1` -XX:+TieredCompilation -client
>>
>> Also instead of number for error code can you use messages? It will be
>> very difficult to trace which test failed by number especially since you
>> use expressions for it.
>>
>> On 4/16/13 9:06 AM, Igor Ignatyev wrote:
>>>  > Okay. Please, test it: generate replay.txt with C1 without recording
>>>  > comp_level (old style) and then try to replay it with new code.
>>> 'TestVM_no_comp_level.sh' emulates old style of replay data.
>>
>> Okay.
>>
>>>
>>>  >>> Add tests for tiered with different -XX:TieredStopAtLevel=n.
>>>  >> what exactly did you mean?
>>>  >> 1) generate replay.txt by VM with different value of
>>> 'TieredStopAtLevel'
>>>  >> 2) use different 'TieredStopAtLevel' during replay of compilation
>>>  >
>>>  > I meant 1). VM could crash at any level of compilation because they
>>> are
>>>  > different so we need to be able replay all of them. So I want to
>>> verify
>>>  > that it works (positive tests).
>>> block 'if [ $is_tiered -eq 1 ]' was added to TestVM.sh
>>
>> Thanks.
>>
>>>
>>>> Run in JPRT to verify that tests work on all platforms.
>>> There are some failures in TestSA.sh due to known bugs in SA
>>> (JDK-8011888, JDK-6326653) and environment/infra issue on macosx (core
>>> wasn't generated because '/cores' directory doesn't exist or isn't
>>> writable by current user[1]).
>>
>> We should disable core files generation and tests on Mac OS. The Nightly
>> should be clean.
>>
>> Thanks,
>> Vladimir
>>
>>>
>>> updated webrev: http://cr.openjdk.java.net/~iignatyev/8011675/webrev.04/
>>>
>>> [1]
>>> http://developer.apple.com/library/mac/technotes/tn2124/_index.html#//apple_ref/doc/uid/DTS10003391-CH1-SECCOREDUMPS
>>>
>>>
>>>
>>>
>>> Best regards,
>>> Igor Ignatyev
>>>
>>> On 04/16/2013 01:35 AM, Vladimir Kozlov wrote:
>>>> On 4/13/13 12:14 PM, Igor Ignatyev wrote:
>>>>>> If you replaying C1 compilation next setting will be incorrect.
>>>>>> Should
>>>>>> we use CompLevel_highest_tier instead so that following check pass?:
>>>>>>
>>>>>> +     if (had_error() && (error_message() == comp_level_label)) {
>>>>>> +       comp_level = CompLevel_full_optimization;
>>>>>>
>>>>>
>>>>> i added this code for compatible with old version and i think that it
>>>>> must has the same behavior (use 'CompLevel_full_optimization'). in new
>>>>> version we always will have comp_level in replay.txt.
>>>>> so i think we shouldn't use CompLevel_highest_tier.
>>>>
>>>> Okay. Please, test it: generate replay.txt with C1 without recording
>>>> comp_level (old style) and then try to replay it with new code.
>>>>
>>>>>
>>>>>> Add tests for tiered with different -XX:TieredStopAtLevel=n.
>>>>> what exactly did you mean?
>>>>> 1) generate replay.txt by VM with different value of
>>>>> 'TieredStopAtLevel'
>>>>> 2) use different 'TieredStopAtLevel' during replay of compilation
>>>>
>>>> I meant 1). VM could crash at any level of compilation because they are
>>>> different so we need to be able replay all of them. So I want to verify
>>>> that it works (positive tests).
>>>>
>>>>>
>>>>> if 1st, i don't think that test must to iterate through all possible
>>>>> values, it must be done by harness, test execution etc
>>>>
>>>> I agree with you but, unfortunately, we are short on testing
>>>> hardware to
>>>> add an other testing variation. You should know. So adding this testing
>>>> here when tiered VM is tested will help us now.
>>>>
>>>>> if 2nd, current implementation of ciReplay ignore TieredStopAtLevel
>>>>> flags, so these tests will be useless.
>>>>>
>>>>>> You may still not find core file. Or ulimit -c unlimited will not
>>>>>> work
>>>>>> (windows, OS X?). You need to bailout in such case as success.
>>>>> for windows i added '-XX:+CreateMinidumpOnCrash', but whatever case of
>>>>> not found core-file need to treat as a environment issue, a test
>>>>> bug or
>>>>> may even be a product bug. anyway if it happened, i think it would be
>>>>> better to know about this than just skip the test.
>>>>>
>>>>> i updated webrev according to the rest of your suggestions, but i'm
>>>>> not
>>>>> sure about flags to limit core size.
>>>>
>>>> Run in JPRT to verify that tests work on all platforms.
>>>>
>>>>>
>>>>> also i added one more check of comp_level and moved validation to
>>>>> separate method.
>>>>
>>>> OK.
>>>>
>>>> Thanks,
>>>> Vladimir
>>>>
>>>>>
>>>>> http://cr.openjdk.java.net/~iignatyev/8011675/webrev.03/
>>>>>
>>>>> Best regards,
>>>>> Igor Ignatyev
>>>>>
>>>>> On 04/13/2013 02:14 AM, Vladimir Kozlov wrote:
>>>>>> I would set CrashAfterCompileId default to -1 as indication it is not
>>>>>> set. Then you don't need to check CrashAfterCompileId == 0 since
>>>>>> compile_id is positive. By the way, it is very nice idea but add a
>>>>>> comment about what that code for. I did not get it first :)
>>>>>>
>>>>>> About name. For such flags we use CI: CIBreakAt, CIStop. So I would
>>>>>> suggest to use CICrashAt. Which means VM crash with that compile id,
>>>>>> not
>>>>>> when it is greater.
>>>>>>
>>>>>> If you replaying C1 compilation next setting will be incorrect.
>>>>>> Should
>>>>>> we use CompLevel_highest_tier instead so that following check pass?:
>>>>>>
>>>>>> +     if (had_error() && (error_message() == comp_level_label)) {
>>>>>> +       comp_level = CompLevel_full_optimization;
>>>>>>
>>>>>>
>>>>>> About tests.
>>>>>>
>>>>>> Add -Xmx32m, -XX:ParallelGCThreads=2 and other flags (codecache
>>>>>> ,metaspace) to limit core size on big machines.
>>>>>>
>>>>>> Add tests for tiered with different -XX:TieredStopAtLevel=n.
>>>>>>
>>>>>> I would add more info in output why the test is skipped:
>>>>>>       echo TEST SKIPPED
>>>>>>
>>>>>> typo 'java':
>>>>>>   124 is_tiered=`${JAVA} ${TESTVMOPTS} java -XX:+PrintFlagsFinal
>>>>>> -version 2>&1 | grep TieredCompilation | grep -c true`
>>>>>>
>>>>>>
>>>>>> I don't like next:
>>>>>>
>>>>>>   130 ${JAVA} ${TESTVMOPTS} -Xcomp -XX:CrashAfterCompileId=1
>>>>>> -XX:+CreateMinidumpOnCrash -XX:+DumpReplayDataOnError -version &
>>>>>>   131 crash_pid=$!
>>>>>>   132 wait $crash_pid
>>>>>>
>>>>>>   On windows 'wait $crash_pid' may not work (happened before) because
>>>>>> before you execute 'wait' OS may assign the same pid to different
>>>>>> process.
>>>>>>
>>>>>> You may still not find core file. Or ulimit -c unlimited will not
>>>>>> work
>>>>>> (windows, OS X?). You need to bailout in such case as success. Next
>>>>>> code
>>>>>> is incorrect for that:
>>>>>>
>>>>>>    48 if [ -z "${core_file}" -o ! -r "${core_file}" ]
>>>>>>    49 then
>>>>>>    50     echo TEST FAILED: core wasn\'t generated
>>>>>>    51     exit 2
>>>>>>    52 fi
>>>>>>
>>>>>> Thanks
>>>>>> Vladimir
>>>>>>
>>>>>> On 4/12/13 8:27 AM, Igor Ignatyev wrote:
>>>>>>> updated webrev:
>>>>>>> http://cr.openjdk.java.net/~iignatyev/8011675/webrev.02/
>>>>>>> added:
>>>>>>>    1. more informative message on unsupported comp_level
>>>>>>>    2. not-product flag 'CrashAfterCompileId' and assert in
>>>>>>> compileBroker.cpp for assured generation of replay.txt
>>>>>>>    2. tests for ciReplay
>>>>>>>
>>>>>>> Best regards,
>>>>>>> Igor Ignatyev
>>>>>>>
>>>>>>> On 04/09/2013 11:40 AM, Vladimir Kozlov wrote:
>>>>>>>> Igor,
>>>>>>>>
>>>>>>>> Sorry, I may said it not clear. What I meant is to set error (call
>>>>>>>> report_error()) in such case and return from process_compile()
>>>>>>>> without
>>>>>>>> compilation. But let ciReplay::replay() finish and exit.
>>>>>>>> replay_impl()
>>>>>>>> should print the problem so you don't need to do it in
>>>>>>>> process_compile(). And, please, test it.
>>>>>>>> The message should be more informative, something like:
>>>>>>>> "compilation level %d requires TieredCompilation"
>>>>>>>> An other bad cases is when failure happened in Client (C1) or
>>>>>>>> Server
>>>>>>>> (C2) but the replay is run with opposite VM.
>>>>>>>>
>>>>>>>> thanks,
>>>>>>>> Vladimir
>>>>>>>>
>>>>>>>> On 4/9/13 12:04 AM, Igor Ignatyev wrote:
>>>>>>>>> Vladimir,
>>>>>>>>>
>>>>>>>>> Thank you for review.
>>>>>>>>>
>>>>>>>>> updated webrev:
>>>>>>>>> http://cr.openjdk.java.net/~iignatyev/8011675/webrev.01/
>>>>>>>>>
>>>>>>>>> Best regards,
>>>>>>>>> Igor Ignatyev
>>>>>>>>>
>>>>>>>>> On 04/09/2013 04:19 AM, Vladimir Kozlov wrote:
>>>>>>>>>> Igor,
>>>>>>>>>>
>>>>>>>>>> Thank you for fixing and cleaning this.
>>>>>>>>>>
>>>>>>>>>> In vmStructs.cpp move _comp_level after _compile_id as in nmethod
>>>>>>>>>> class.
>>>>>>>>>>
>>>>>>>>>> Typo in ciReplay.cpp:
>>>>>>>>>> +      comp_level != CompLevel_highest_tier;
>>>>>>>>>>
>>>>>>>>>> Actually it is useless to replay compilation at different
>>>>>>>>>> level, it
>>>>>>>>>> should exit with error. Could happened because incorrectly run
>>>>>>>>>> without
>>>>>>>>>> Tiered when the crush happened with Tiered.
>>>>>>>>>>
>>>>>>>>>> In ciReplay.cpp use parenthesis around equality checks: if (. &&
>>>>>>>>>> (. ==
>>>>>>>>>> .)).
>>>>>>>>>>
>>>>>>>>>> Thanks,
>>>>>>>>>> Vladimir
>>>>>>>>>>
>>>>>>>>>> On 4/8/13 12:30 PM, Igor Ignatyev wrote:
>>>>>>>>>>> Hi all,
>>>>>>>>>>>
>>>>>>>>>>> Please review patch.
>>>>>>>>>>>
>>>>>>>>>>> Problem:
>>>>>>>>>>> Replay of compilations supports c1 and tiered as well as c2, the
>>>>>>>>>>> only
>>>>>>>>>>> thing missing is comp_level.
>>>>>>>>>>>
>>>>>>>>>>> Fix:
>>>>>>>>>>> 1. added comp_level into dump and usage during replay
>>>>>>>>>>> 2. minor changes in agent/doc:
>>>>>>>>>>>    - escaped '<', '>'
>>>>>>>>>>>    - renamed 'C2 Replay'/'C2 compiler replay' to
>>>>>>>>>>> 'Replay'/'Compiler
>>>>>>>>>>> replay'
>>>>>>>>>>>
>>>>>>>>>>> Testing:
>>>>>>>>>>> manually launched the replay from JDK-8010934 (assert
>>>>>>>>>>> specifically to
>>>>>>>>>>> c1) on jvm w/o corresponded fix. assert was triggered w/ patch
>>>>>>>>>>> and
>>>>>>>>>>> wasn't triggered w/o.
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>> webrev: http://cr.openjdk.java.net/~iignatyev/8011675/webrev.00/
>>>>>>>>>>> jbs: https://jbs.oracle.com/bugs/browse/JDK-8011675

More information about the hotspot-compiler-dev mailing list