rednaxelafx commented on issue #25710: [SPARK-29008][SQL] Define an individual method for each common subexpression in HashAggregateExec URL: https://github.com/apache/spark/pull/25710#issuecomment-529042370 I haven't reviewed this PR in detail yet, just some first thoughts: At a glance I'm neutral about this PR. In general I don't like code splitting that cause premature spilling of state from locals to fields. I might be more in favor of: ```java long commonSubExpr0 = agg_subExpr_0(input1, input2); agg_doAggregate_sum_0(commonSubExpr0); ... ``` than ```java agg_subExpr_0(input1, input2); // result goes to this.commonSubExpr0 agg_doAggregate_sum_0(); // argument passed through `this` ... ``` In practice, after thorough inlining, the performance shouldn't be too different, but I just don't like the idea of blindly spilling state to fields when it's not necessary. JIT compilers can optimize code, but usually has a hard time optimizing field layout and removing "unnecessary" fields -- that requires strong inter-procedural analysis.
---------------------------------------------------------------- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: [email protected] With regards, Apache Git Services --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
