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

Igor Ignatyev igor.ignatyev at oracle.com
Thu Apr 18 13:48:35 PDT 2013 

Vladimir,
thank you for review.

i have additional small changes for it:
- improved finding of coredump location, now it correctly works on mac os.

http://cr.openjdk.java.net/~iignatyev/8011675/webrev.06/

Best regards,
Igor Ignatyev

On 04/17/2013 09:36 PM, Vladimir Kozlov wrote:
> 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