[jira] [Comment Edited] (GROOVY-10657) Generated code for overloaded assignment operator prevents garbage collection

["Eric Milles (Jira)" <ji...@apache.org>]

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

Eric Milles edited comment on GROOVY-10657 at 6/14/22 2:28 PM:
---------------------------------------------------------------

As we process the assignment expression `employees["1"] = "Bob"` we have to deal with the RHS to understand what it is (see below).  Just after this, we need to stash the top operand into a temp var so that we can push `employees` and `"1"` onto the stack.  Then the RHS is pushed back.  At this point, the temp could be discarded I suppose.

From {{BinaryExpressionHelper}}:
{code:java}
    public void evaluateEqual(final BinaryExpression expression, final boolean defineVariable) {
        AsmClassGenerator acg = controller.getAcg();
        CompileStack compileStack = controller.getCompileStack();
        OperandStack operandStack = controller.getOperandStack();
        Expression leftExpression = expression.getLeftExpression();
        Expression rightExpression = expression.getRightExpression();
        boolean directAssignment = defineVariable && !(leftExpression instanceof TupleExpression);

        if (directAssignment && rightExpression instanceof EmptyExpression) {
            VariableExpression ve = (VariableExpression) leftExpression;
            BytecodeVariable var = compileStack.defineVariable(ve, controller.getTypeChooser().resolveType(ve, controller.getClassNode()), false);
            operandStack.loadOrStoreVariable(var, false);
            return;
        }
        // evaluate the RHS and store the result
        // TODO: LHS has not been visited, it could be a variable in a closure and type chooser is not aware.
        ClassNode lhsType = controller.getTypeChooser().resolveType(leftExpression, controller.getClassNode());
        if (rightExpression instanceof ListExpression && lhsType.isArray()) {
            ListExpression list = (ListExpression) rightExpression;
            ArrayExpression array = new ArrayExpression(lhsType.getComponentType(), list.getExpressions());
            array.setSourcePosition(list);
            array.visit(acg);
        } else if (rightExpression instanceof EmptyExpression) {
            loadInitValue(leftExpression.getType()); // TODO: lhsType?
        } else {
            rightExpression.visit(acg);
        }

        ClassNode rhsType = operandStack.getTopOperand();
        int rhsValueId = compileStack.defineTemporaryVariable("$rhs", rhsType, true);
{code}


was (Author: emilles):
As we process the assignment expression `employees["1"] = "Bob"` we have to deal with the RHS to understand what it is (see below).  Just after this, we need to stash the top operand into a temp var so that we can push `employees` and `"1"` onto the stack.  Then the RHS is pushed back.  At this point, the temp could be discarded I suppose.

From {{BinaryExpressionHelper}}:
{code:java}
    public void evaluateEqual(final BinaryExpression expression, final boolean defineVariable) {
        AsmClassGenerator acg = controller.getAcg();
        CompileStack compileStack = controller.getCompileStack();
        OperandStack operandStack = controller.getOperandStack();
        Expression leftExpression = expression.getLeftExpression();
        Expression rightExpression = expression.getRightExpression();
        boolean directAssignment = defineVariable && !(leftExpression instanceof TupleExpression);

        if (directAssignment && rightExpression instanceof EmptyExpression) {
            VariableExpression ve = (VariableExpression) leftExpression;
            BytecodeVariable var = compileStack.defineVariable(ve, controller.getTypeChooser().resolveType(ve, controller.getClassNode()), false);
            operandStack.loadOrStoreVariable(var, false);
            return;
        }
        // evaluate the RHS and store the result
        // TODO: LHS has not been visited, it could be a variable in a closure and type chooser is not aware.
        ClassNode lhsType = controller.getTypeChooser().resolveType(leftExpression, controller.getClassNode());
        if (rightExpression instanceof ListExpression && lhsType.isArray()) {
            ListExpression list = (ListExpression) rightExpression;
            ArrayExpression array = new ArrayExpression(lhsType.getComponentType(), list.getExpressions());
            array.setSourcePosition(list);
            array.visit(acg);
        } else if (rightExpression instanceof EmptyExpression) {
            loadInitValue(leftExpression.getType()); // TODO: lhsType?
        } else {
            rightExpression.visit(acg);
        }

        ClassNode rhsType = operandStack.getTopOperand();
        int rhsValueId;
{code}

> 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
>            Priority: Major
>
> 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
(v8.20.7#820007)