[ 
https://issues.apache.org/jira/browse/GROOVY-11955?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18075574#comment-18075574
 ] 

ASF GitHub Bot commented on GROOVY-11955:
-----------------------------------------

codecov-commenter commented on PR #2485:
URL: https://github.com/apache/groovy/pull/2485#issuecomment-4300511724

   ## 
[Codecov](https://app.codecov.io/gh/apache/groovy/pull/2485?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 `93.75000%` with `1 line` in your changes missing 
coverage. Please review.
   :white_check_mark: Project coverage is 67.0568%. Comparing base 
([`9df1d73`](https://app.codecov.io/gh/apache/groovy/commit/9df1d739d376280d2db2e4366f18eca3050902ff?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache))
 to head 
([`4d7c1e3`](https://app.codecov.io/gh/apache/groovy/commit/4d7c1e37702b239271d105789268a5c24359960a?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
   
   | [Files with missing 
lines](https://app.codecov.io/gh/apache/groovy/pull/2485?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
 | Patch % | Lines |
   |---|---|---|
   | 
[.../groovy/classgen/asm/indy/InvokeDynamicWriter.java](https://app.codecov.io/gh/apache/groovy/pull/2485?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Forg%2Fcodehaus%2Fgroovy%2Fclassgen%2Fasm%2Findy%2FInvokeDynamicWriter.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9vcmcvY29kZWhhdXMvZ3Jvb3Z5L2NsYXNzZ2VuL2FzbS9pbmR5L0ludm9rZUR5bmFtaWNXcml0ZXIuamF2YQ==)
 | 93.7500% | [0 Missing and 1 partial :warning: 
](https://app.codecov.io/gh/apache/groovy/pull/2485?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>
   
   
   
   [![Impacted file tree 
graph](https://app.codecov.io/gh/apache/groovy/pull/2485/graphs/tree.svg?width=650&height=150&src=pr&token=1r45138NfQ&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)](https://app.codecov.io/gh/apache/groovy/pull/2485?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
   
   ```diff
   @@                Coverage Diff                 @@
   ##               master      #2485        +/-   ##
   ==================================================
   + Coverage     67.0522%   67.0568%   +0.0046%     
   - Complexity      31533      31534         +1     
   ==================================================
     Files            1451       1451                
     Lines          122424     122423         -1     
     Branches        21956      21963         +7     
   ==================================================
   + Hits            82088      82093         +5     
   + Misses          33260      33248        -12     
   - Partials         7076       7082         +6     
   ```
   
   | [Files with missing 
lines](https://app.codecov.io/gh/apache/groovy/pull/2485?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
 | Coverage Δ | |
   |---|---|---|
   | 
[.../groovy/classgen/asm/indy/InvokeDynamicWriter.java](https://app.codecov.io/gh/apache/groovy/pull/2485?src=pr&el=tree&filepath=src%2Fmain%2Fjava%2Forg%2Fcodehaus%2Fgroovy%2Fclassgen%2Fasm%2Findy%2FInvokeDynamicWriter.java&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-c3JjL21haW4vamF2YS9vcmcvY29kZWhhdXMvZ3Jvb3Z5L2NsYXNzZ2VuL2FzbS9pbmR5L0ludm9rZUR5bmFtaWNXcml0ZXIuamF2YQ==)
 | `89.6342% <93.7500%> (-4.9967%)` | :arrow_down: |
   
   ... and [5 files with indirect coverage 
changes](https://app.codecov.io/gh/apache/groovy/pull/2485/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>




> Provide a size guard for flattening of chained method calls
> -----------------------------------------------------------
>
>                 Key: GROOVY-11955
>                 URL: https://issues.apache.org/jira/browse/GROOVY-11955
>             Project: Groovy
>          Issue Type: Bug
>            Reporter: Daniel Kuppitz
>            Assignee: Paul King
>            Priority: Major
>             Fix For: 6.0.0-alpha-1, 5.0.6
>
>
> The chained call method flattening we added in GROOVY-7785 generates slightly 
> slower bytecode and slightly larger bytecode. This isn't necessarily a 
> problem for most code but for frameworks like Spock which inject quite a bit 
> of code when converting assertions to their value-recording form, it can 
> reduce the number of assertions that can appear in a method before hitting 
> JVM limits.
> Credit to Juho Naalisvaara on Slack for providing the example (made up 
> example to show an issue they were having in their real tests):
> {code}
> static def generateSpec(def count) {
>     def assertions = (1..count).collect { i ->
>         "assert obj.field${i % 10} == 'value${i % 10}'"
>     }.join("\n\t\t\t\t")
>     def spec = """\
> @Grab('org.spockframework:spock-core:2.4-groovy-5.0')
> import spock.lang.Specification
> class MethodTooLargeReproducerSpec extends Specification {
>     def "verifyAll with ${count} assertions"() {
>         given:
>             def obj = [field0: 'value0', field1: 'value1', field2: 'value2',
>                        field3: 'value3', field4: 'value4', field5: 'value5',
>                        field6: 'value6', field7: 'value7', field8: 'value8',
>                        field9: 'value9']
>         expect:
>             verifyAll {
>             \t${assertions}
>             }
>     }
> }
> """
> }
> def gcl = new GroovyClassLoader()
> (589..597).step(1).each { count ->
>     def src = generateSpec(count)
>     try {
>         gcl.parseClass(src)
>         println "OK:     ${count}"
>     } catch (Exception e) {
>         println "FAILED: ${count} — ${e.message}"
>         System.exit(1)
>     }
> }
> {code}
> This code bombs out with an error like this:
> {noformat}
> FAILED: 589 — startup failed:
> General error during instruction selection: Method too large: 
> MethodTooLargeReproducerSpec$__spock_feature_0_0_closure1.doCall 
> (Ljava/lang/Object;)Ljava/lang/Object;
> groovyjarjarasm.asm.MethodTooLargeException: Method too large: 
> MethodTooLargeReproducerSpec$__spock_feature_0_0_closure1.doCall 
> (Ljava/lang/Object;)Ljava/lang/Object;
>       at 
> groovyjarjarasm.asm.MethodWriter.computeMethodInfoSize(MethodWriter.java:2088)
> ...
> {noformat}
> For Groovy 5.0.4 you could have 595 assertions (with similar complexity to 
> above) in a method before hitting this limit. In Groovy 5.0.5 you could only 
> have 585. A small decrease, and yes workarounds like splitting the method 
> come to mind, but I propose to add a guard so we do the flattening only when 
> chaining depth gets above a threshold.
> In addition, the flattened bytecode is very slightly less performant, so this 
> will be a win for code not needing the unrolling. We know the stackoverflow 
> territory covered by GROOVY-7785 is 500-1000 calls deep, so I was going to 
> propose a threshold of 64. We can adjust if we don't find that gives us best 
> performance.



--
This message was sent by Atlassian Jira
(v8.20.10#820010)

Reply via email to