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

[Igor Ignatyev]

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

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

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

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