[
https://issues.apache.org/jira/browse/GROOVY-12162?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18097115#comment-18097115
]
ASF GitHub Bot commented on GROOVY-12162:
-----------------------------------------
daniellansun commented on code in PR #2707:
URL: https://github.com/apache/groovy/pull/2707#discussion_r3604178197
##########
src/test/groovy/org/codehaus/groovy/classgen/asm/PeepholeOptimizingMethodVisitorTest.groovy:
##########
@@ -366,6 +441,788 @@ final class PeepholeOptimizingMethodVisitorTest {
]
}
+ @Test
+ void preservesStandaloneCheckcastBeforeReturn() {
+ def bytecode = sequenceFor('(Ljava/lang/Object;)V') {
+ visitVarInsn(Opcodes.ALOAD, 0)
+ visitInsn(Opcodes.NOP)
+ visitTypeInsn(Opcodes.CHECKCAST, 'java/lang/String')
+ visitInsn(Opcodes.POP)
+ visitInsn(RETURN)
+ }
+
+ // Standalone CHECKCAST before POP is dropped; before RETURN it is
flushed
+ // and the value is popped so the void return remains verifiable.
+ def returnOnly = sequenceFor('(Ljava/lang/Object;)V') {
+ visitVarInsn(Opcodes.ALOAD, 0)
+ visitInsn(Opcodes.NOP)
+ visitTypeInsn(Opcodes.CHECKCAST, 'java/lang/String')
+ visitInsn(RETURN)
+ }
+
+ assert opcodeLines(bytecode) == ['ALOAD 0', 'NOP', 'POP', 'RETURN']
+ assert opcodeLines(returnOnly) == ['ALOAD 0', 'NOP', 'CHECKCAST
java/lang/String', 'POP', 'RETURN']
+ }
+
+ @Test
+ void preservesAttachedCheckcastSideEffectBeforeReturn() {
+ def bytecode = sequenceFor('(Ljava/lang/Object;)V') {
+ visitVarInsn(Opcodes.ALOAD, 0)
+ visitTypeInsn(Opcodes.CHECKCAST, 'java/lang/String')
+ visitInsn(RETURN)
+ }
+
+ // Cast is kept for its ClassCastException side effect; POP keeps void
return valid.
+ assert opcodeLines(bytecode) == [
+ 'ALOAD 0',
+ 'CHECKCAST java/lang/String',
+ 'POP',
+ 'RETURN',
+ ]
+ }
+
+ @Test
+ void wrapIsIdempotentAndPropagatesNull() {
+ assert PeepholeOptimizingMethodVisitor.wrap(null) == null
+
+ def methodNode = new MethodNode(CompilerConfiguration.ASM_API_VERSION,
ACC_PUBLIC | ACC_STATIC, 'sample', '()V', null, null)
+ def once = PeepholeOptimizingMethodVisitor.wrap(methodNode)
+ assert once instanceof PeepholeOptimizingMethodVisitor
+ assert PeepholeOptimizingMethodVisitor.wrap(once).is(once)
+ }
+
+ @Test
+ void printTraceBytecodeFindsNestedTracer() {
+ def textifier = new Textifier()
+ def tracer = new TraceMethodVisitor(textifier)
+ def peephole = new PeepholeOptimizingMethodVisitor(tracer)
+ peephole.visitCode()
+ peephole.visitInsn(Opcodes.ICONST_1)
+ peephole.visitVarInsn(Opcodes.ISTORE, 0) // keep the constant live for
tracing
+ peephole.visitInsn(RETURN)
+ peephole.visitMaxs(0, 0)
+ peephole.visitEnd()
+
+ def out = new StringWriter()
+ def printer = new PrintWriter(out)
+ assert PeepholeOptimizingMethodVisitor.printTraceBytecode(peephole,
printer)
+ printer.flush()
+ assert out.toString().contains('ICONST_1')
+ assert out.toString().contains('ISTORE')
+ assert out.toString().contains('RETURN')
+
+ assert !PeepholeOptimizingMethodVisitor.printTraceBytecode(null,
printer)
+ assert !PeepholeOptimizingMethodVisitor.printTraceBytecode(new
MethodNode(CompilerConfiguration.ASM_API_VERSION, ACC_PUBLIC | ACC_STATIC, 'x',
'()V', null, null), printer)
+ }
+
+ @Test
+ void classVisitorWrapsEveryMethodAndSkipsNullDelegates() {
+ def written = []
+ def delegate = new
org.objectweb.asm.ClassVisitor(CompilerConfiguration.ASM_API_VERSION) {
+ @Override
+ MethodVisitor visitMethod(int access, String name, String
descriptor, String signature, String[] exceptions) {
+ if (name == 'skip') {
+ return null
+ }
+ def mn = new MethodNode(CompilerConfiguration.ASM_API_VERSION,
access, name, descriptor, signature, exceptions)
+ written << mn
+ return mn
+ }
+ }
+ def peepholeClass = new PeepholeOptimizingClassVisitor(delegate)
+
+ def optimized = peepholeClass.visitMethod(ACC_PUBLIC | ACC_STATIC,
'run', '()V', null, null)
+ assert optimized instanceof PeepholeOptimizingMethodVisitor
+ optimized.visitCode()
+ optimized.visitLdcInsn(0)
+ optimized.visitVarInsn(Opcodes.ISTORE, 0)
+ optimized.visitInsn(RETURN)
+ optimized.visitMaxs(0, 0)
+ optimized.visitEnd()
+
+ assert peepholeClass.visitMethod(ACC_PUBLIC | ACC_STATIC, 'skip',
'()V', null, null) == null
+ assert written.size() == 1
+ assert opcodeLines(traceSequence(written[0])) == ['ICONST_0', 'ISTORE
0', 'RETURN']
+ }
+
+ @Test
+ void doesNotRewriteNonMatchingCompareJumps() {
+ def zeroLabel = new Label()
+ def zeroWithIfnull = sequenceFor('(I)V') {
+ visitVarInsn(Opcodes.ILOAD, 0)
+ visitLdcInsn(0)
+ visitJumpInsn(Opcodes.IFNULL, zeroLabel)
+ visitInsn(RETURN)
+ visitLabel(zeroLabel)
+ visitInsn(RETURN)
+ }
+ assert opcodeLines(zeroWithIfnull)[1] == 'ICONST_0'
+ assert opcodeLines(zeroWithIfnull)[2].startsWith('IFNULL')
+
+ def nullLabel = new Label()
+ def nullWithIfeq = sequenceFor('(Ljava/lang/Object;)V') {
+ visitVarInsn(Opcodes.ALOAD, 0)
+ visitInsn(Opcodes.ACONST_NULL)
+ visitJumpInsn(Opcodes.IFEQ, nullLabel)
+ visitInsn(RETURN)
+ visitLabel(nullLabel)
+ visitInsn(RETURN)
+ }
+ assert opcodeLines(nullWithIfeq)[1] == 'ACONST_NULL'
+ assert opcodeLines(nullWithIfeq)[2].startsWith('IFEQ')
+ }
+
+ @Test
+ void preservesMismatchedPopSizes() {
+ def bytecode = sequenceFor('(IJ)V') {
+ visitVarInsn(Opcodes.ILOAD, 0)
+ visitInsn(Opcodes.POP2)
+ visitVarInsn(Opcodes.LLOAD, 1)
+ visitInsn(Opcodes.POP)
+ visitInsn(RETURN)
+ }
+
+ assert opcodeLines(bytecode) == [
+ 'ILOAD 0',
+ 'POP2',
+ 'LLOAD 1',
+ 'POP',
+ 'RETURN',
+ ]
+ }
+
+ @Test
+ void attachesCheckcastToReferenceConstantsAndDropsDeadCastChains() {
+ def type = org.objectweb.asm.Type.getType(String)
+ def handle = new Handle(Opcodes.H_INVOKESTATIC, 'Owner', 'boot',
'()V', false)
+ def bytecode = sequenceFor {
+ visitLdcInsn('text')
+ visitTypeInsn(Opcodes.CHECKCAST, 'java/lang/String')
+ visitInsn(Opcodes.POP)
+ visitLdcInsn(type)
+ visitTypeInsn(Opcodes.CHECKCAST, 'java/lang/Class')
+ visitInsn(Opcodes.POP)
+ visitLdcInsn(handle)
+ visitTypeInsn(Opcodes.CHECKCAST, 'java/lang/invoke/MethodHandle')
+ visitInsn(Opcodes.POP)
+ visitInsn(Opcodes.ACONST_NULL)
+ visitTypeInsn(Opcodes.CHECKCAST, 'java/lang/String')
+ visitInsn(Opcodes.POP)
+ visitInsn(RETURN)
+ }
+
+ assert opcodeLines(bytecode) == ['RETURN']
+ }
+
+ @Test
+ void flushesBeforeMethodAndInvokeDynamicCalls() {
+ def handle = new Handle(Opcodes.H_INVOKESTATIC, 'Owner', 'bootstrap',
'()Ljava/lang/invoke/CallSite;', false)
+ def bytecode = sequenceFor('(Ljava/lang/Object;)V') {
+ visitVarInsn(Opcodes.ALOAD, 0)
+ visitMethodInsn(Opcodes.INVOKEVIRTUAL, 'java/lang/Object',
'toString', '()Ljava/lang/String;', false)
+ visitInsn(Opcodes.POP)
+ visitLdcInsn(1)
+ visitInvokeDynamicInsn('dyn', '()I', handle)
+ visitInsn(Opcodes.POP)
+ visitInsn(RETURN)
+ }
+
+ def lines = opcodeLines(bytecode)
+ assert lines[0] == 'ALOAD 0'
+ assert lines[1] == 'INVOKEVIRTUAL java/lang/Object.toString
()Ljava/lang/String;'
+ assert lines[2] == 'POP'
+ assert lines[3] == 'ICONST_1'
+ assert lines[4].startsWith('INVOKEDYNAMIC dyn')
+ assert lines[-2] == 'POP'
+ assert lines[-1] == 'RETURN'
+ }
+
+ @Test
+ void flushesPendingLoadOnLineNumberAndNonCheckcastTypeInsn() {
+ def start = new Label()
+ def bytecode = sequenceFor('(Ljava/lang/Object;)V') {
+ visitLabel(start)
+ visitVarInsn(Opcodes.ALOAD, 0)
+ visitLineNumber(42, start)
+ visitTypeInsn(Opcodes.INSTANCEOF, 'java/lang/String')
+ visitInsn(Opcodes.POP)
+ visitInsn(RETURN)
+ }
+
+ assert opcodeLines(bytecode) == [
+ 'ALOAD 0',
+ 'INSTANCEOF java/lang/String',
+ 'POP',
+ 'RETURN',
+ ]
+ }
+
+ @Test
+ void doesNotAttachSecondCheckcastAndFlushesIincOnConflict() {
+ def chainedCasts =
sequenceFor('(Ljava/lang/Object;)Ljava/lang/Object;') {
+ visitVarInsn(Opcodes.ALOAD, 0)
+ visitTypeInsn(Opcodes.CHECKCAST, 'java/lang/CharSequence')
+ visitTypeInsn(Opcodes.CHECKCAST, 'java/lang/String')
+ visitInsn(Opcodes.ARETURN)
+ }
+ assert opcodeLines(chainedCasts) == [
+ 'ALOAD 0',
+ 'CHECKCAST java/lang/CharSequence',
+ 'CHECKCAST java/lang/String',
+ 'ARETURN',
+ ]
+
+ def secondIincFlushes = sequenceFor('(I)V') {
+ visitVarInsn(Opcodes.ILOAD, 0)
+ visitIincInsn(0, 1)
+ visitIincInsn(0, 2) // already has IINC → flush load+first IINC,
emit second
+ visitInsn(Opcodes.POP)
+ visitInsn(RETURN)
+ }
+ assert opcodeLines(secondIincFlushes) == [
+ 'ILOAD 0',
+ 'IINC 0 1',
+ 'IINC 0 2',
+ 'POP',
+ 'RETURN',
+ ]
+
+ def differentVarIincFlushes = sequenceFor('(I)V') {
+ visitVarInsn(Opcodes.ILOAD, 0)
+ visitIincInsn(1, 1) // different variable → flush load, emit IINC
+ visitInsn(Opcodes.POP)
+ visitInsn(RETURN)
+ }
+ assert opcodeLines(differentVarIincFlushes) == [
+ 'ILOAD 0',
+ 'IINC 1 1',
+ 'POP',
+ 'RETURN',
+ ]
+ }
+
+ @Test
+ void flushesPendingDupWhenStoreIsNotEligibleForCollapse() {
+ def bytecode = sequenceFor {
+ visitInsn(Opcodes.ICONST_1)
+ visitInsn(Opcodes.DUP)
+ visitFieldInsn(Opcodes.PUTFIELD, 'Owner', 'value', 'I') // not
PUTSTATIC → flush dup
+ visitInsn(Opcodes.ICONST_2)
+ visitInsn(Opcodes.DUP)
+ visitVarInsn(Opcodes.ISTORE, 0)
+ visitInsn(Opcodes.NOP) // keep the duplicate
+ visitInsn(RETURN)
+ }
+
+ assert opcodeLines(bytecode) == [
+ 'ICONST_1',
+ 'DUP',
+ 'PUTFIELD Owner.value : I',
+ 'ICONST_2',
+ 'DUP',
+ 'ISTORE 0',
+ 'NOP',
+ 'RETURN',
+ ]
+ }
+
+ @Test
+ void passesConstantDynamicThroughWithoutBuffering() {
+ def handle = new Handle(Opcodes.H_INVOKESTATIC, 'Owner', 'bootstrap',
'()I', false)
+ def dynamic = new ConstantDynamic('answer', 'I', handle)
+ def bytecode = sequenceFor {
+ visitLdcInsn(dynamic)
+ visitInsn(Opcodes.POP)
+ visitInsn(RETURN)
+ }
+
+ // Bootstrap side effects must not be dropped by dead-load elimination.
+ def lines = opcodeLines(bytecode)
+ assert lines.size() == 3
+ assert lines[0].startsWith('LDC')
+ assert lines[1] == 'POP'
+ assert lines[2] == 'RETURN'
+ }
+
+ @Test
+ void emitsNonSpecializedNumericConstantsViaLdc() {
+ def bytecode = sequenceFor {
+ visitLdcInsn(3L)
+ visitLdcInsn(4f)
+ visitLdcInsn(5d)
+ visitLdcInsn('literal')
+ visitVarInsn(Opcodes.ASTORE, 0)
+ visitInsn(RETURN)
+ }
+
+ assert opcodeLines(bytecode) == [
+ 'LDC 3L',
+ 'LDC 4.0F',
+ 'LDC 5.0D',
+ 'LDC "literal"',
+ 'ASTORE 0',
+ 'RETURN',
+ ]
+ }
+
+ @Test
+ void passesThroughNewarrayIntInsn() {
+ def bytecode = sequenceFor {
+ visitLdcInsn(2)
+ visitIntInsn(Opcodes.NEWARRAY, Opcodes.T_INT)
+ visitInsn(Opcodes.POP)
+ visitInsn(RETURN)
+ }
+
+ assert opcodeLines(bytecode) == [
+ 'ICONST_2',
+ 'NEWARRAY T_INT',
+ 'POP',
+ 'RETURN',
+ ]
+ }
+
+ @Test
+ void dropsDeadFloatAndDoubleLoads() {
+ def bytecode = sequenceFor('(FD)V') {
+ visitVarInsn(Opcodes.FLOAD, 0)
+ visitInsn(Opcodes.POP)
+ visitVarInsn(Opcodes.DLOAD, 1)
+ visitInsn(Opcodes.POP2)
+ visitInsn(Opcodes.FCONST_0)
+ visitInsn(Opcodes.POP)
+ visitInsn(Opcodes.DCONST_0)
+ visitInsn(Opcodes.POP2)
+ visitInsn(RETURN)
+ }
+
+ assert opcodeLines(bytecode) == ['RETURN']
+ }
+
+ @Test
+ void collapsesDup2WithWideVariableStore() {
+ def bytecode = sequenceFor {
+ visitLdcInsn(9L)
+ visitInsn(Opcodes.DUP2)
+ visitVarInsn(Opcodes.LSTORE, 0)
+ visitInsn(Opcodes.POP2)
+ visitLdcInsn(8.0d)
+ visitInsn(Opcodes.DUP2)
+ visitVarInsn(Opcodes.DSTORE, 2)
+ visitInsn(Opcodes.POP2)
+ visitInsn(RETURN)
+ }
+
+ assert opcodeLines(bytecode) == [
+ 'LDC 9L',
+ 'LSTORE 0',
+ 'LDC 8.0D',
+ 'DSTORE 2',
+ 'RETURN',
+ ]
+ }
+
+ @Test
+ void collapsesDupStorePopForReferenceStaticField() {
+ def bytecode = sequenceFor {
+ visitLdcInsn('value')
+ visitInsn(Opcodes.DUP)
+ visitFieldInsn(Opcodes.PUTSTATIC, 'Owner', 'NAME',
'Ljava/lang/String;')
+ visitInsn(Opcodes.POP)
+ visitInsn(RETURN)
+ }
+
+ assert opcodeLines(bytecode) == [
+ 'LDC "value"',
+ 'PUTSTATIC Owner.NAME : Ljava/lang/String;',
+ 'RETURN',
+ ]
+ }
+
+ @Test
+ void preservesMismatchedDupAndPopSizes() {
+ def bytecode = sequenceFor {
+ visitInsn(Opcodes.ICONST_1)
+ visitInsn(Opcodes.DUP)
+ visitInsn(Opcodes.POP2) // size mismatch → keep DUP
+ visitLdcInsn(2L)
+ visitInsn(Opcodes.DUP2)
+ visitVarInsn(Opcodes.ISTORE, 0) // wide dup + narrow store → no
collapse on later pop
+ visitInsn(Opcodes.POP2)
+ visitInsn(RETURN)
+ }
+
+ assert opcodeLines(bytecode) == [
+ 'ICONST_1',
+ 'DUP',
+ 'POP2',
+ 'LDC 2L',
+ 'DUP2',
+ 'ISTORE 0',
+ 'POP2',
+ 'RETURN',
+ ]
+ }
+
+ @Test
+ void dropsDeadLoadWithAttachedCheckcastOnPopOnly() {
+ def withPop = sequenceFor('(Ljava/lang/Object;)V') {
+ visitLdcInsn('x')
+ visitTypeInsn(Opcodes.CHECKCAST, 'java/lang/String')
+ visitInsn(Opcodes.POP)
+ visitInsn(RETURN)
+ }
+ assert opcodeLines(withPop) == ['RETURN']
+
+ def standalonePop2 = sequenceFor('(Ljava/lang/Object;)V') {
+ visitVarInsn(Opcodes.ALOAD, 0)
+ visitInsn(Opcodes.NOP)
+ visitTypeInsn(Opcodes.CHECKCAST, 'java/lang/String')
+ visitInsn(Opcodes.POP2) // only POP removes standalone cast
+ visitInsn(RETURN)
+ }
+ assert opcodeLines(standalonePop2) == [
+ 'ALOAD 0',
+ 'NOP',
+ 'CHECKCAST java/lang/String',
+ 'POP2',
+ 'RETURN',
+ ]
+ }
+
+ // -
> Harden and extend bytecode peephole optimization
> ------------------------------------------------
>
> Key: GROOVY-12162
> URL: https://issues.apache.org/jira/browse/GROOVY-12162
> Project: Groovy
> Issue Type: Improvement
> Reporter: Daniel Sun
> Priority: Major
>
> h2. Summary
> Follow-up to GROOVY-12065. Make the single-pass peephole layer apply to
> *every* method body, restore classgen diagnostics after visitor wrapping, and
> extend safe stack-local rewrites so generated bytecode stays compact without
> changing runtime semantics.
> h2. Background
> After GROOVY-12065, {{OperandStack}} emits many constants uniformly via
> {{visitLdcInsn}} and relies on {{PeepholeOptimizingMethodVisitor}} to narrow
> them ({{ICONST_*}}, {{BIPUSH}}, {{SIPUSH}}, etc.).
> Two practical gaps remained:
> # *Incomplete coverage* — the peephole wrapper was applied only at selected
> {{visitMethod}} sites. Synthetic helpers (call-site array init, large-list
> chunks, MOP bridges, …) often bypassed it and kept non-compact forms.
> # *Broken diagnostics* — with the peephole visitor outside
> {{TraceMethodVisitor}}, {{mv instanceof TraceMethodVisitor}} failed, so "Last
> known generated bytecode" was never printed when {{visitMaxs}} failed under
> classgen logging.
> h2. Goals
> * One installation point so *all* methods (user + synthetic) get the same
> compaction.
> * Correct classgen logging / failure dumps through the wrapper chain.
> * More rewrites that stay single-pass, stack-local, and semantics-preserving.
> * Strong unit coverage for edge cases (CHECKCAST side effects, void
> {{RETURN}}, signed zeros, box/unbox mismatch).
> h2. Approach
> h3. Uniform installation
> * Add {{PeepholeOptimizingClassVisitor}}, installed once from
> {{WriterController}}, wrapping every method visitor.
> * Remove ad-hoc {{new PeepholeOptimizingMethodVisitor(...)}} from
> {{AsmClassGenerator}} and {{CallSiteWriter}}.
> * {{PeepholeOptimizingMethodVisitor.wrap(null)}} propagates {{null}}
> unchanged (ASM may skip a method body); wrap is idempotent.
> h3. Diagnostics
> * Add {{PeepholeOptimizingMethodVisitor.printTraceBytecode}} to walk nested
> peephole layers and print an inner {{TraceMethodVisitor}} when present.
> * {{AsmClassGenerator}} uses it on {{visitMaxs}} failure so classgen logging
> still shows the last known bytecode.
> h3. Extended rewrites (single basic-block window)
> Still at most one pending load, one pending box, and one pending
> {{DUP}}/store:
> * *CHECKCAST attachment* — attach to pending {{ALOAD}} or reference constant
> so {{load}}; {{CHECKCAST}}; {{POP}} can collapse when unused; keep the cast
> when the value is used.
> * *Null compare* — {{ACONST_NULL}}; {{IF_ACMPEQ}}/{{IF_ACMPNE}} →
> {{IFNULL}}/{{IFNONNULL}}.
> * *Zero compare* — {{ICONST_0}}; {{IF_ICMP*}} → {{IF*}} via the fixed JVM
> opcode offset ({{IF_ICMPEQ}}…{{IF_ICMPLE}} ↔ {{IFEQ}}…{{IFLE}}).
> * *Bare DUP / DUP+store+pop* — matching {{POP}}/{{POP2}} collapses to a plain
> store, or eliminates the dup entirely.
> * *Void RETURN* — drop dead loads/constants (preserve paired {{IINC}}); for
> pending {{CHECKCAST}} (standalone or attached), emit the cast then {{POP}} so
> the type-check side effect is kept and the void return stays verifiable.
> * *Constants* — integers re-emitted via {{BytecodeHelper.pushConstant}} on
> the *delegate* (no re-buffering); {{ConstantDynamic}} is never buffered
> (bootstrap may have side effects); signed FP zeros still compared by raw bits
> (GROOVY-9797).
> * *Box/unbox cancellation* (gjit-inspired) — matching {{Wrapper.valueOf}} +
> {{xxxValue}}, or {{DefaultTypeTransformation.box}} + {{xxxUnbox}}, cancel to
> a no-op; mismatched types are left intact.
> * *Dead box* — {{valueOf}}/{{box}} + {{POP}}/{{POP2}} drops the box and pops
> the original primitive ({{POP2}} when wide).
> * *Boolean constant folding* — {{GETSTATIC Boolean.TRUE/FALSE}} +
> {{booleanValue}}/{{booleanUnbox}} → {{ICONST_1}}/{{ICONST_0}}.
> h2. Safety
> * Flush pending state before labels, frames, line numbers, invokes (except
> matched box/unbox), switches, maxs/end, and other non-local boundaries.
> * No second compilation pass; no full data-flow analysis.
> * Observable behaviour unchanged; only denser equivalent bytecode within a
> basic block.
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
