Inline withRetryIfBlocking and the typed equality operators

[olabusayoT]

withRetryIfBlocking evaluates its body in place, since a blocked evaluation's RetryableException already propagates to the suspension machinery that retries it later. Making it inline means the by-name body no longer allocates a closure on every call, and the separate helper that wrapped it is removed.

The typed equality operators (=#=, !=#=, =:=, !=:=, eq, ne) are inline so each call site compares using its operands' static types and expands in place. As ordinary methods they compared through the erased type and went through boxed equality on every call. The implicit TypeEquality is now only the compile-time proof that the two types are related.

Behavior is unchanged.

(Pulled from the prefetch PR to simplify review #1736 )

DAFFODIL-3065

[stevedlawrence]

+1, seems there's still some bytecode related to typed equality. It's probably not a big deal, and I thin it is an improvement, but something to consider. Also suggest we just remove withRetryIfBlocking entirely since it doesn't actually do anything.

[stevedlawrence]

The file TestEqualityOperators.scala has an object with this comment:

/**
 * By looking at the byte code for the methods of this
 * object, one can determine whether these typed equality operators
 * are allocating or not, and what code is generated.
 */
object TestEqualityOperators {

  def compare(x: String, y: String) = {
    x =:= y
  }

  def compareIntLong(x: Int, y: Long) = {
    x.toLong =#= y
  }
}

So doing that and looking at the decompiled code for these, we get this which has referenced to TypeEquality andViewEqual that makes me think it's not inlining or only partially inlineing:

  public boolean compare(java.lang.String, java.lang.String);
    Code:
       0: getstatic     #42                 // Field org/apache/daffodil/lib/equality/package$.MODULE$:Lorg/apache/daffodil/lib/equality/package$;
       3: aload_1
       4: invokevirtual #46                 // Method org/apache/daffodil/lib/equality/package$.TypeEqual:(Ljava/lang/Object;)Ljava/lang/Object;
       7: checkcast     #48                 // class java/lang/String
      10: astore_3
      11: getstatic     #51                 // Field org/apache/daffodil/lib/equality/package$TypeEquality$.MODULE$:Lorg/apache/daffodil/lib/equality/package$TypeEquality$;
      14: invokevirtual #55                 // Method org/apache/daffodil/lib/equality/package$TypeEquality$.rightSubtypeOfLeftEquality:()Lorg/apache/daffodil/lib/equality/package$TypeEquality;
      17: astore        4
      19: aload_3
      20: aload_2
      21: astore        5
      23: dup
      24: ifnonnull     36
      27: pop
      28: aload         5
      30: ifnull        44
      33: goto          48
      36: aload         5
      38: invokevirtual #59                 // Method java/lang/Object.equals:(Ljava/lang/Object;)Z
      41: ifeq          48
      44: iconst_1
      45: goto          49
      48: iconst_0
      49: ireturn

  public boolean compareIntLong(int, long);
    Code:
       0: getstatic     #42                 // Field org/apache/daffodil/lib/equality/package$.MODULE$:Lorg/apache/daffodil/lib/equality/package$;
       3: iload_1
       4: i2l
       5: invokestatic  #71                 // Method scala/runtime/BoxesRunTime.boxToLong:(J)Ljava/lang/Long;
       8: invokevirtual #74                 // Method org/apache/daffodil/lib/equality/package$.ViewEqual:(Ljava/lang/Object;)Ljava/lang/Object;
      11: checkcast     #76                 // class java/lang/Long
      14: astore        4
      16: aload         4
      18: invokestatic  #80                 // Method scala/runtime/BoxesRunTime.unboxToLong:(Ljava/lang/Object;)J
      21: lload_2
      22: lcmp
      23: ifne          30
      26: iconst_1
      27: goto          31
      30: iconst_0
      31: ireturn

If I change those two functions to use normal non-typed equality I get this:

  public boolean compare(java.lang.String, java.lang.String);
    Code:
       0: aload_1
       1: aload_2
       2: astore_3
       3: dup
       4: ifnonnull     15
       7: pop
       8: aload_3
       9: ifnull        22
      12: goto          26
      15: aload_3
      16: invokevirtual #33                 // Method java/lang/Object.equals:(Ljava/lang/Object;)Z
      19: ifeq          26
      22: iconst_1
      23: goto          27
      26: iconst_0
      27: ireturn

  public boolean compareIntLong(int, long);
    Code:
       0: iload_1
       1: i2l
       2: lload_2
       3: lcmp
       4: ifne          11
       7: iconst_1
       8: goto          12
      11: iconst_0
      12: ireturn

Which is more along the lines of what I would expect.

Note that I get different results on main, so I think this is likely an improvement, but there still might be changes needed to get rid all the overhead.

[olabusayoT]

Should we, in a separate PR, updates other @inline annotations in the code, and change them to inline;also implicits, see below

+--------------------------------+------------------------+--------------------------------------------------+
|        Scala 2 Feature         |  Scala 3 Replacement   |                  Primary Intent                  |
+--------------------------------+------------------------+--------------------------------------------------+
| implicit val / implicit object | given instance         | Defining context parameters/type class instances |
| implicit parameter list        | using clause           | Requiring context parameters                     |
| implicit class                 | extension block        | Adding new methods to existing types             |
| implicit def                   | given Conversion[A, B] | Coercing one type into another automatically     |
+--------------------------------+------------------------+--------------------------------------------------+

[stevedlawrence]

Yeah, we should look at our uses of @inline and see if we should switch to inline. Can be done in a separate PR.

I'm not familiar enough with the scala 3 replacements but we probably should look into update our code to use what Scala 3 recommends. We definitely don't plan to move Daffodil back to scala 2 so maintaining support isn't a concern.

[stevedlawrence]

If we keep the withRetryIfBlocking function, I would recommend we comment out the old version so it acts as a sort of history of what it's intention is, so we ever revisit this there's a history of what we had tried.

Or if we just want to remove withRetryIfBlocking entirely (probably reasonable, I don't expect we'll ever bring it back), we should also remove the function along with the old version and the commented out code at the top of the function talking about thisExpressionCoroutine_, coroutinToResumeIfBlocked_, etc.