[GitHub] spark pull request #16648: [SPARK-18016][SQL][CATALYST] Code Generation: Con...

[kiszk <gi...@git.apache.org>]

Github user kiszk commented on a diff in the pull request:

    https://github.com/apache/spark/pull/16648#discussion_r117602817
  
    --- Diff: sql/catalyst/src/main/scala/org/apache/spark/sql/catalyst/expressions/codegen/CodeGenerator.scala ---
    @@ -145,11 +145,85 @@ class CodegenContext {
        *
        * They will be kept as member variables in generated classes like `SpecificProjection`.
        */
    -  val mutableStates: mutable.ArrayBuffer[(String, String, String)] =
    -    mutable.ArrayBuffer.empty[(String, String, String)]
    +  val mutableState: mutable.ListBuffer[(String, String, String)] =
    +    mutable.ListBuffer.empty[(String, String, String)]
     
    -  def addMutableState(javaType: String, variableName: String, initCode: String): Unit = {
    -    mutableStates += ((javaType, variableName, initCode))
    +  // An array keyed by the tuple of mutable states' types and initialization codes, holds the
    +  // current max index of the array
    +  var mutableStateArrayIdx: mutable.Map[(String, String), Int] =
    +    mutable.Map.empty[(String, String), Int]
    +
    +  // An array keyed by the tuple of mutable states' types and initialization codes, holds the name
    +  // of the mutableStateArray into which state of the given key will be compacted
    +  var mutableStateArrayNames: mutable.Map[(String, String), String] =
    +    mutable.Map.empty[(String, String), String]
    +
    +  // An array keyed by the tuple of mutable states' types and initialization codes, holds the code
    +  // that will initialize the mutableStateArray when initialized in loops
    +  var mutableStateArrayInitCodes: mutable.Map[(String, String), String] =
    +    mutable.Map.empty[(String, String), String]
    +
    +  /**
    +   * Adds an instance of globally-accessible mutable state. Mutable state may either be inlined
    +   * as a private member variable to the class, or it may be compacted into arrays of the same
    +   * type and initialization in order to avoid Constant Pool limit errors for both state declaration
    +   * and initialization.
    +   *
    +   * We compact state into arrays when we can anticipate variables of the same type and initCode
    +   * may appear numerous times. Variable names with integer suffixes (as given by the `freshName`
    +   * function), that are either simply assigned (null or no initialization) or are primitive are
    +   * good candidates for array compaction, as these variables types are likely to appear numerous
    +   * times, and can be easily initialized in loops.
    +   *
    +   * @param javaType the javaType
    +   * @param variableName the variable name
    +   * @param initCode the initialization code for the variable
    +   * @return the name of the mutable state variable, which is either the original name if the
    +   *         variable is inlined to the class, or an array access if the variable is to be stored
    +   *         in an array of variables of the same type and initialization.
    +   */
    +  def addMutableState(
    --- End diff --
    
    Thank you for updating generated code. It makes clear.
    
    When I see this method, `addMutableState` returns an array element if a variable is primitive or simple object even if the number of mutable states are small. Does to always use array element lead to performance overhead compared to using instance variables?


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---

---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@spark.apache.org
For additional commands, e-mail: reviews-help@spark.apache.org