[
https://issues.apache.org/jira/browse/GROOVY-10657?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18140818#comment-18140818
]
ASF GitHub Bot commented on GROOVY-10657:
-----------------------------------------
blackdrag commented on code in PR #3226:
URL: https://github.com/apache/groovy/pull/3226#discussion_r4506990204
##########
src/main/java/org/codehaus/groovy/classgen/asm/BinaryExpressionMultiTypeDispatcher.java:
##########
@@ -433,21 +440,57 @@ protected void assignToArray(final Expression orig, final
Expression receiver, f
// load rhs
rhsValueLoader.visit(asmGenerator);
+ // The store consumes the value. Duplicate it under the array
reference
+ // and the int index first, so the assignment expression can yield
the
+ // original value without a local variable. The copy is taken
before the
+ // component cast, which applies to the consumed value only.
+ if
(!Boolean.TRUE.equals(orig.getNodeMetaData(AsmClassGenerator.ELIDE_EXPRESSION_VALUE)))
{
+ duplicateTopBeneath(operandStack, 2);
+ }
operandStack.doGroovyCast(arrayType.getComponentType());
// store value in array
bew.arraySet(false);
- // update operand stack
+ // update operand stack; a duplicated value stays as the
expression result
operandStack.remove(3);
-
- if
(!Boolean.TRUE.equals(orig.getNodeMetaData(AsmClassGenerator.ELIDE_EXPRESSION_VALUE)))
- rhsValueLoader.visit(asmGenerator); // re-load result value
} else {
super.assignToArray(orig, receiver, index, rhsValueLoader, safe);
}
}
+ /**
+ * Duplicates the top operand beneath {@code operandsBelow} tracked
operands.
+ * An array store has an array reference and an int index below the value,
+ * two words in total; the value is one word, or two for long and double.
+ */
+ private void duplicateTopBeneath(final OperandStack operandStack, final
int operandsBelow) {
+ ClassNode top = operandStack.getTopOperand();
+ operandStack.remove(1);
+ ClassNode[] below = new ClassNode[operandsBelow];
+ int wordsBelow = 0;
+ for (int i = operandsBelow - 1; i >= 0; i -= 1) {
+ below[i] = operandStack.getTopOperand();
+ wordsBelow += isTwoWord(below[i]) ? 2 : 1;
+ operandStack.remove(1);
+ }
+ int wordsTop = isTwoWord(top) ? 2 : 1;
+ int opcode;
+ if (wordsTop == 1 && wordsBelow == 1) opcode = DUP_X1;
+ else if (wordsTop == 1 && wordsBelow == 2) opcode = DUP_X2;
+ else if (wordsTop == 2 && wordsBelow == 1) opcode = DUP2_X1;
+ else if (wordsTop == 2 && wordsBelow == 2) opcode = DUP2_X2;
Review Comment:
this should be in OperandStack (see dup and swap)
##########
src/test/groovy/org/codehaus/groovy/classgen/asm/sc/StaticCompilationTest.groovy:
##########
@@ -261,21 +262,73 @@ final class StaticCompilationTest extends
AbstractBytecodeTestCase {
'L0',
'LINENUMBER 3',
'ALOAD 1',
- 'ASTORE 2',
'ICONST_0',
- 'ISTORE 3',
'ICONST_1',
- 'ISTORE 4',
- 'ALOAD 2',
- 'ILOAD 3',
- 'ILOAD 4',
'INVOKESTATIC
org/codehaus/groovy/runtime/BytecodeInterface8.intArraySet ([III)V',
'L1',
'LINENUMBER 4',
'RETURN'
])
}
+ // GROOVY-10657: a reference right-hand side must not be kept in a local
+ @Test
+ void testSubscriptAssignmentReference() {
+ assert compile(method: 'm', '''@groovy.transform.CompileStatic
+ void m(Object[] a, Object v) {
+ a[0] = v
+ }
+ ''').hasStrictSequence([
+ 'L0',
+ 'LINENUMBER 3',
+ 'ALOAD 1',
+ 'ICONST_0',
+ 'ALOAD 2',
+ 'INVOKESTATIC
org/codehaus/groovy/runtime/BytecodeInterface8.objectArraySet
([Ljava/lang/Object;ILjava/lang/Object;)V',
+ 'L1',
+ 'LINENUMBER 4',
+ 'RETURN'
+ ])
+ }
+
+ // GROOVY-10657: the assignment value is the right-hand side, duplicated
under the store operands
+ @Test
+ void testSubscriptAssignmentValue() {
+ assert compile(method: 'm', '''@groovy.transform.CompileStatic
+ Object m(Object[] a, Object v) {
+ return (a[0] = v)
+ }
+ ''').hasStrictSequence([
+ 'L0',
+ 'LINENUMBER 3',
+ 'ALOAD 1',
+ 'ICONST_0',
+ 'ALOAD 2',
+ 'DUP_X2',
+ 'INVOKESTATIC
org/codehaus/groovy/runtime/BytecodeInterface8.objectArraySet
([Ljava/lang/Object;ILjava/lang/Object;)V',
+ 'ARETURN'
+ ])
+ }
+
+ // GROOVY-10657: a category-2 value uses DUP2_X2 so the copy keeps both
words
+ @Test
+ void testSubscriptAssignmentLongValue() {
Review Comment:
actually most of these test should be not only testing static compilation,
but also have that kind of sequence without static compilation.
##########
src/main/java/org/codehaus/groovy/classgen/asm/BinaryExpressionMultiTypeDispatcher.java:
##########
@@ -433,21 +440,57 @@ protected void assignToArray(final Expression orig, final
Expression receiver, f
// load rhs
rhsValueLoader.visit(asmGenerator);
+ // The store consumes the value. Duplicate it under the array
reference
+ // and the int index first, so the assignment expression can yield
the
+ // original value without a local variable. The copy is taken
before the
+ // component cast, which applies to the consumed value only.
+ if
(!Boolean.TRUE.equals(orig.getNodeMetaData(AsmClassGenerator.ELIDE_EXPRESSION_VALUE)))
{
+ duplicateTopBeneath(operandStack, 2);
+ }
operandStack.doGroovyCast(arrayType.getComponentType());
// store value in array
bew.arraySet(false);
- // update operand stack
+ // update operand stack; a duplicated value stays as the
expression result
operandStack.remove(3);
-
- if
(!Boolean.TRUE.equals(orig.getNodeMetaData(AsmClassGenerator.ELIDE_EXPRESSION_VALUE)))
- rhsValueLoader.visit(asmGenerator); // re-load result value
} else {
super.assignToArray(orig, receiver, index, rhsValueLoader, safe);
}
}
+ /**
+ * Duplicates the top operand beneath {@code operandsBelow} tracked
operands.
+ * An array store has an array reference and an int index below the value,
+ * two words in total; the value is one word, or two for long and double.
+ */
+ private void duplicateTopBeneath(final OperandStack operandStack, final
int operandsBelow) {
Review Comment:
The only value for operandsBelow seems to be 2, why do we need this to work
for another case?
> Generated code for overloaded assignment operator prevents garbage collection
> -----------------------------------------------------------------------------
>
> Key: GROOVY-10657
> URL: https://issues.apache.org/jira/browse/GROOVY-10657
> Project: Groovy
> Issue Type: Bug
> Components: Compiler
> Affects Versions: 4.0.3
> Reporter: Jonathan Baxter
> Assignee: Daniel Sun
> Priority: Major
> Fix For: 7.0.0-alpha-1, 6.0.1
>
>
> The groovy code
> {code:java}
> obj["lhs"] = "rhs"{code}
> ostensibly converts to
> {code:java}
> obj.putAt("lhs","rhs")
> {code}
> if {{obj}} has the appropriate {{putAt}} method.
> However , in the process of tracking down a memory leak, I discovered that
> assignment is not exactly equivalent to invocation of the {{putAt}} method.
> Specifically, the code generated by the compiler is equivalent to creating an
> additional temporary variable to hold the rhs of the assignment, and then
> invoking the {{putAt}} method with that temporary variable as its second
> argument:
> {code:java}
> String string = "rhs"
> obj.putAt("lhs",string){code}
> The problem with this is that since {{"rhs"}} now has a live reference
> ({{{}string{}}}), it is not eligible for garbage collection.
> In my particular use case, {{"rhs"}} is a very large object that I would like
> to be automatically garbage collected after the assignment is executed, but
> before the script has finished. But that means I have to use the underlying
> {{putAt}} method rather than the assignment operator, which rather negates
> the benefits of overloading the assignment operator.
> One potential fix would be to simply null out any temporary variables
> constructed by the compiler, so that
> {code:java}
> obj["lhs"] = "rhs"
> {code}
> becomes
> {code:java}
> String string = "rhs"
> obj.putAt("lhs",string)
> string = null <----- "rhs" is now eligible for garbage-collection{code}
--
This message was sent by Atlassian Jira
(v10.3.26#10030026)
