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

Vladimir Kozlov vladimir.kozlov at oracle.com
Mon Apr 22 08:08:37 PDT 2013 

Looks good.

thanks,
Vladimir

On 4/22/13 6:58 AM, Igor Ignatyev wrote:
> I have rerun JPRT:
>   - TestSA.sh passed on macosx
>   - TestSA.sh failed only due to known bugs in SA.
>   - other ciReplay tests passed
>
> Best regards,
> Igor Ignatyev
>
> On 04/19/2013 08:38 PM, Igor Ignatyev wrote:
>>> Can you save command's output (stderr and stdout) into a file and grep
>>> it instead?
>> i did it, even i don't see any reason for it. could you explain the
>> cause of your request?
>>
>> http://cr.openjdk.java.net/~iignatyev/8011675/webrev.07/
>>
>> Best regards,
>> Igor Ignatyev
>>
>> On 04/19/2013 02:25 AM, Vladimir Kozlov wrote:
>>> I wish we also record core's location in hs_err file so you can grep it
>>> instead of stderr output.
>>>
>>> Can you save command's output (stderr and stdout) into a file and grep
>>> it instead?
>>>
>>> Thanks,
>>> Vladimir
>>>
>>> On 4/18/13 1:48 PM, Igor Ignatyev wrote:
>>>> 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