Inlined code shouldn't forget null-check on the original receiver

[scabug]

Running the program below should result in NPE, however under -optimize the println statement is executed.

object Test {
  def main(args: Array[String]): Unit = {
    val r: C = null;
    r.callee()
  }
}

final class C {
  @inline final def callee() { println("callee's body has run.") }
}

@VladUreche, watch out for a similar situation when rewriting tail calls.

[scabug]

Imported From: https://issues.scala-lang.org/browse/SI-5850?orig=1
Reporter: @magarciaEPFL

[scabug]

@magarciaEPFL said:
Two design alternatives:

• null-check and conditionally throwing NPE, takes 8 bytes in opcodes (snippet 1 below)
• factoring out that functionality into a newly added ScalaRunTime.nullCheck(arg), takes 3 bytes.

I'm favoring the second alternative.

Just for the record, the snippets for each alternative:

/ /Addition_to_Inliners.scala
val NPEClass = definitions.NullPointerExceptionClass
val nwNPE    = NEW(REFERENCE(NPEClass))
val npeInit  = definitions.getMember(NPEClass, nme.CONSTRUCTOR).suchThat(_.paramss == List(Nil))
assert(npeInit != NoSymbol, "Couldn't find the no-args constructor of NullPointerException.")
val cmNPEInit = CALL_METHOD(npeInit, Static(true))
nwNPE.init    = cmNPEInit

val npeBlock = newBlock()
npeBlock.emit(nwNPE,                    instr.pos)
npeBlock.emit(DUP(REFERENCE(NPEClass)), instr.pos)
npeBlock.emit(cmNPEInit,                instr.pos)
npeBlock.emit(THROW(NPEClass),          instr.pos)
npeBlock.close()

// Addition_to_ScalaRunTime.scala
def nullCheck(x: AnyRef) {
  if (x == null) throw new NullPointerException
}

[scabug]

@magarciaEPFL said:
A variation on alternative 2 above: add nullCheck(Object) to src\library\scala\runtime\Statics.java

[scabug]

@ijuma said:
The best scenario is to do the null check inline, but only throw the exception in a separate method. That way you move the cold path into a separate method.

[scabug]

@magarciaEPFL said:
@ijuma, that helps somewhat but performance still suffers unless the inliner gets smarter about which null-checks are redundant. The following shows that: https://github.com/magarciaEPFL/scala/tree/SI-5850

The type-flow analysis should track non-nullness. I was planning to update the TFA anyway (to also track whether a type is exact, ie after new). Only then can this bug be closed.

[scabug]

@ijuma said:
Interesting. Would you mind sharing your numbers for each alternative you tried?

In theory, null checks are free, but there is the added bytecode size. See:

"Null checks are cheap. They usually fold straight into a related memory access instruction, and use the CPU bus logic to catch nulls. (Deoptimization follows, with regenerated code containing an explicit check.)
User-written null checks are in most cases functionally identical to those inserted by the JVM.
Null checks can be hoisted manually, and suppress implicit null checks in dominated blocks."

https://wikis.oracle.com/display/HotSpotInternals/PerformanceTechniques

[scabug]

@magarciaEPFL said:
Yes, increased code size is the problem. The heuristics built into scalac's inliner are very sensitive to them, preventing inlinings currently done. The non-redundant null-checks, once in place, will most likely be infrequent enough not to increase code size nearly as much. We'll know for sure after the #5849 and #5852 pending "improvements".

[scabug]

@ijuma said:
Thanks for the explanation.

[scabug]

@adriaanm said:
this would also resolve part of the problem reported in #6941 and #6508

[scabug]

@JamesIry said:
2.10.2 is about to be cut. Kicking down the road and un-assigning to foster work stealing.

[SethTisue]

the println doesn't run in 2.12