[
https://issues.apache.org/jira/browse/GROOVY-12542?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18133371#comment-18133371
]
ASF GitHub Bot commented on GROOVY-12542:
-----------------------------------------
codecov-commenter commented on PR #3062:
URL: https://github.com/apache/groovy/pull/3062#issuecomment-6460161133
##
[Codecov](https://app.codecov.io/gh/apache/groovy/pull/3062?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
Report
:x: Patch coverage is `88.82353%` with `19 lines` in your changes missing
coverage. Please review.
:white_check_mark: Project coverage is 73.6276%. Comparing base
([`cfd9b2f`](https://app.codecov.io/gh/apache/groovy/commit/cfd9b2f0222a8e1a74473f7c5b2cb45b5bdd3b5b?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache))
to head
([`32a5321`](https://app.codecov.io/gh/apache/groovy/commit/32a5321a8be2faf54f5e44f98a5cea3c75b9ac81?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 2 commits behind head on master.
| [Files with missing
lines](https://app.codecov.io/gh/apache/groovy/pull/3062?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
| Patch % | Lines |
|---|---|---|
|
[...ssgen/asm/ConditionalJumpFoldingMethodVisitor.java](https://app.codecov.io/gh/apache/groovy/pull/3062?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Forg%2Fcodehaus%2Fgroovy%2Fclassgen%2Fasm%2FConditionalJumpFoldingMethodVisitor.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9vcmcvY29kZWhhdXMvZ3Jvb3Z5L2NsYXNzZ2VuL2FzbS9Db25kaXRpb25hbEp1bXBGb2xkaW5nTWV0aG9kVmlzaXRvci5qYXZh)
| 88.4848% | [14 Missing and 5 partials :warning:
](https://app.codecov.io/gh/apache/groovy/pull/3062?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
|
<details><summary>Additional details and impacted files</summary>
[](https://app.codecov.io/gh/apache/groovy/pull/3062?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
```diff
@@ Coverage Diff @@
## master #3062 +/- ##
==================================================
+ Coverage 73.6047% 73.6276% +0.0229%
+ Complexity 42348 42172 -176
==================================================
Files 1657 1658 +1
Lines 145708 145876 +168
Branches 27498 27512 +14
==================================================
+ Hits 107248 107405 +157
- Misses 28479 28490 +11
Partials 9981 9981
```
| [Files with missing
lines](https://app.codecov.io/gh/apache/groovy/pull/3062?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
| Coverage Δ | |
|---|---|---|
|
[...y/classgen/asm/PeepholeOptimizingClassVisitor.java](https://app.codecov.io/gh/apache/groovy/pull/3062?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Forg%2Fcodehaus%2Fgroovy%2Fclassgen%2Fasm%2FPeepholeOptimizingClassVisitor.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9vcmcvY29kZWhhdXMvZ3Jvb3Z5L2NsYXNzZ2VuL2FzbS9QZWVwaG9sZU9wdGltaXppbmdDbGFzc1Zpc2l0b3IuamF2YQ==)
| `100.0000% <100.0000%> (ø)` | |
|
[.../classgen/asm/PeepholeOptimizingMethodVisitor.java](https://app.codecov.io/gh/apache/groovy/pull/3062?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Forg%2Fcodehaus%2Fgroovy%2Fclassgen%2Fasm%2FPeepholeOptimizingMethodVisitor.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9vcmcvY29kZWhhdXMvZ3Jvb3Z5L2NsYXNzZ2VuL2FzbS9QZWVwaG9sZU9wdGltaXppbmdNZXRob2RWaXNpdG9yLmphdmE=)
| `86.1757% <100.0000%> (+0.0538%)` | :arrow_up: |
|
[...ssgen/asm/ConditionalJumpFoldingMethodVisitor.java](https://app.codecov.io/gh/apache/groovy/pull/3062?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Forg%2Fcodehaus%2Fgroovy%2Fclassgen%2Fasm%2FConditionalJumpFoldingMethodVisitor.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9vcmcvY29kZWhhdXMvZ3Jvb3Z5L2NsYXNzZ2VuL2FzbS9Db25kaXRpb25hbEp1bXBGb2xkaW5nTWV0aG9kVmlzaXRvci5qYXZh)
| `88.4848% <88.4848%> (ø)` | |
... and [9 files with indirect coverage
changes](https://app.codecov.io/gh/apache/groovy/pull/3062/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
</details>
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test
Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests,
report on failures, and find test suite problems.
- :package: [JS Bundle
Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save
yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
> Branch directly on comparisons in conditions instead of first producing a
> boolean value
> ---------------------------------------------------------------------------------------
>
> Key: GROOVY-12542
> URL: https://issues.apache.org/jira/browse/GROOVY-12542
> Project: Groovy
> Issue Type: Improvement
> Reporter: Paul King
> Priority: Major
>
> h3. Problem
> A comparison used as an {{if}}/{{while}}/{{for}} condition is first turned
> into a {{0}}/{{1}} value and then branched on, instead of jumping directly as
> javac does:
> {code:groovy}
> @groovy.transform.CompileStatic
> static void m(int n) {
> for (int i = 0; i < n; i++) { }
> }
> {code}
> {noformat}
> Groovy (current) javac (equivalent Java)
> ILOAD i ILOAD i
> ILOAD n ILOAD n
> IF_ICMPGE L1 IF_ICMPGE EXIT
> ICONST_1
> GOTO L2
> L1: ICONST_0
> L2: IFEQ EXIT
> {noformat}
> The comparison code ({{BinaryExpressionWriter.writeStdCompare}} and related
> helpers) always produces a boolean, and the statement writers then emit
> {{IFEQ}}/{{IFNE}} on it. This affects dynamic and statically compiled code.
> The peephole optimizer cannot help, because the pattern spans labels.
> Besides the extra instructions and branches, some bytecode consumers only
> recognise javac's shape. For example, TornadoVM's TC compiler
> ({{CountedLoop}} in tc-ir) requires a loop header to end in {{cond_br (icmp
> iv, bound)}}. With the extra boolean step it finds no loop bound, so
> {{@Parallel}} loops written in Groovy fail with:
> {noformat}
> TC-E3103 the bounds of the @Parallel loops are not compile-time constants
> after specialisation, so no launch domain can be derived
> {noformat}
> h3. Proposed change
> Add a small {{MethodVisitor}} stage, {{ConditionalJumpFoldingMethodVisitor}},
> installed by {{PeepholeOptimizingClassVisitor}} directly under
> {{PeepholeOptimizingMethodVisitor}}. It rewrites:
> {noformat}
> Jcc L1; ICONST_1; GOTO L2; L1: ICONST_0; L2: IFEQ L3 ==> Jcc L3
> Jcc L1; ICONST_1; GOTO L2; L1: ICONST_0; L2: IFNE L3 ==> !Jcc L3
> {noformat}
> for any conditional jump ({{IFxx}}, {{IF_ICMPxx}}, {{IF_ACMPxx}},
> {{IFNULL}}/{{IFNONNULL}}). Partial matches are replayed unchanged.
> Safety:
> * *Exact semantics:* the folded jump tests the same operands, or the same
> {{LCMP}}/{{FCMPx}}/{{DCMPx}} result, as the original, so NaN behaviour is
> unchanged.
> * *Shared labels untouched:* the fold only fires when {{L1}} and {{L2}} are
> each referenced solely by the pattern, so labels shared with other jumps
> (e.g. from {{&&}}/{{||}}) are left as is.
> * *Fails loudly:* if a removed label were ever referenced later, a
> {{GroovyBugError}} is raised rather than emitting wrong bytecode.
> * *Stored values unaffected:* a materialized boolean that is stored rather
> than branched on (e.g. {{boolean r = a < b}}) is not touched.
> * *Frames:* classes are written with {{COMPUTE_FRAMES}}, so no stack map
> frames need adjusting.
> * {{PeepholeOptimizingMethodVisitor.printTraceBytecode}} now unwraps the new
> stage too.
> h3. Tests
> * New {{ConditionalJumpFoldingMethodVisitorTest}}:
> ** bytecode shape for {{for}} and {{if}} conditions
> ** stored comparisons are not folded
> ** runtime results in dynamic and {{@CompileStatic}} code for {{!}}, {{&&}},
> {{||}}, ternary, int/long/float/double, null and identity comparisons
> ** for NaN, a folded branch always agrees with the same comparison stored as
> a value
> * Updated expected bytecode in
> {{StaticCompilationTest.testSingleAssignment3}},
> {{StaticCompileNullCompareOptimizationTest.testOptimizeGroovyTruthForPrimitiveBoolean2}}
> and {{testOptimizeGroovyTruthForPrimitiveNumberType}}, which asserted the
> old shape.
> * Full core test suite: 18,659 tests, 0 failures.
> h3. Note
> Relates to GROOVY-12538 (primitive double/float {{>}} and {{>=}} return true
> for NaN). That fix changes which compare instruction ({{DCMPL}}/{{DCMPG}},
> {{FCMPL}}/{{FCMPG}}) precedes the conditional jump. This change only rewrites
> from the jump onwards, so it preserves whichever instruction is chosen.
> Tested together: with PR #3053 applied on top of this change,
> {{DoubleOperationsTest}} and {{ConditionalJumpFoldingMethodVisitorTest}} both
> pass.
--
This message was sent by Atlassian Jira
(v10.3.26#10030026)
