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

[Igor Ignatyev]

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

> 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

if 1st, i don't think that test must to iterate through all possible 
values, it must be done by harness, test execution etc
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.

also i added one more check of comp_level and moved validation to 
separate method.

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