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

[Vladimir Kozlov]

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