Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
scabug opened this issue May 28, 2012 · 11 comments
Closed

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

scabug opened this issue May 28, 2012 · 11 comments

Comments

@scabug
Copy link

scabug commented May 28, 2012

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
Copy link
Author

scabug commented May 28, 2012

@scabug
Copy link
Author

scabug commented May 29, 2012

@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
Copy link
Author

scabug commented May 29, 2012

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

@scabug
Copy link
Author

scabug commented May 29, 2012

@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
Copy link
Author

scabug commented May 31, 2012

@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
Copy link
Author

scabug commented May 31, 2012

@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
Copy link
Author

scabug commented May 31, 2012

@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
Copy link
Author

scabug commented May 31, 2012

@ijuma said:
Thanks for the explanation.

@scabug
Copy link
Author

scabug commented Jan 23, 2013

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

@scabug
Copy link
Author

scabug commented May 20, 2013

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

@SethTisue
Copy link
Member

the println doesn't run in 2.12

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants