RFR(M): 8067651: Fix Trivial code path for LevelTransitionTest.java
Vladimir Kozlov
vladimir.kozlov at oracle.com
Mon Aug 3 18:50:43 UTC 2020
+1
Thanks,
Vladimir K
On 8/3/20 11:11 AM, Igor Ignatyev wrote:
> Hi Evgeny,
>
> webrev.01 looks good to me, thanks.
>
> -- Igor
>
>> On Aug 3, 2020, at 8:22 AM, Evgeny Nikitin <evgeny.nikitin at oracle.com> wrote:
>>
>> Hi Igor, thanks for review.
>>
>>> - I don't see necessity of move Helper.* methods into the enclosing class, nor do I see it as improving readability of the test. why did you decide to move them?
>>
>> Remnants from the previous developer and their decision :). I personally don't like inner classes and inner helper methods alike, so now I've extracted that into MethodHelper.java. The fact that the methods are used in another test strengthens this decision for me.
>>
>>> - if the test is inapplicable for Xcomp run, you should either throw SkippedException instead of System.err::println at L#67 or use '@requires vm.compMode != "Xcomp"' in jtreg test description. currently, the former provides arguable more clear message that the test wasn't run (as it sets special sub-status which is understood by our test execution system) than the latter (which will just omit test from test results altogether), however @requires is "faster" as jtreg don't need to run any of the test code. in any case, both makes it clean that the test wasn't really performed, while your code will lead to a passed-passed test w/o no automated way to know that the test wasn't run.
>>
>> I choose @requires. Descriptions in most cases are better then in-code logic.
>>
>>> - from you explanation of the fix it's also unclear why BackgroundCompilation got disabled, could you please explain?
>>
>> One of the reasons for the case was uncontrollable switch to another layer in background. I found that switch valuable to make the test behavior predictable.
>>
>> The new webrev: https://cr.openjdk.java.net/~enikitin/8067651/webrev.01/
>>
>> Please review,
>> // Evgeny Nikitin.
>>
>>
>>
>> On 2020-07-31 19:11, Igor Ignatyev wrote:
>>> Hi Evgeny,
>>> in general looks good to me, a couple comments/questions though:
>>> - I don't see necessity of move Helper.* methods into the enclosing class, nor do I see it as improving readability of the test. why did you decide to move them?
>>> - if the test is inapplicable for Xcomp run, you should either throw SkippedException instead of System.err::println at L#67 or use '@requires vm.compMode != "Xcomp"' in jtreg test description. currently, the former provides arguable more clear message that the test wasn't run (as it sets special sub-status which is understood by our test execution system) than the latter (which will just omit test from test results altogether), however @requires is "faster" as jtreg don't need to run any of the test code. in any case, both makes it clean that the test wasn't really performed, while your code will lead to a passed-passed test w/o no automated way to know that the test wasn't run.
>>> - from you explanation of the fix it's also unclear why BackgroundCompilation got disabled, could you please explain?
>>> Thanks,
>>> -- Igor
>>>> On Jul 27, 2020, at 12:38 PM, Evgeny Nikitin <evgeny.nikitin at oracle.com> wrote:
>>>>
>>>> Hi,
>>>>
>>>> Bug: https://bugs.openjdk.java.net/browse/JDK-8067651
>>>> Webrev: https://cr.openjdk.java.net/~enikitin/8067651/webrev.00/
>>>>
>>>> Adjusting the test to current state of the VM.
>>>>
>>>> - Definition of 'trivial code' does not depend on whether the method has been profiled or not;
>>>> - Trivial code does only go level 0 to level 1;
>>>> - Some refactoring.
>>>>
>>>> The change has been checked in mach5 for the 5 platforms (passed).
>>>>
>>>> Please review,
>>>> /Evgeny Nikitin.
>
More information about the hotspot-compiler-dev
mailing list