Skip to content

Speed up .nn #15418

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Jun 15, 2022
Merged

Speed up .nn #15418

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
2c6590d
Speed up `.nn`
odersky Jun 10, 2022
40600f2
Disable inline bytecode test for .nn
odersky Jun 10, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions compiler/test/dotty/tools/backend/jvm/InlineBytecodeTests.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,13 +108,13 @@ class InlineBytecodeTests extends DottyBytecodeTest {
}
}
*/

/*
@Test def inlineNn = {
val source =
s"""
|class Foo {
| def meth1(x: Int | Null): Int = x.nn
| def meth2(x: Int | Null): Int = scala.runtime.Scala3RunTime.nn(x)
| def meth2(x: Int | Null): Int = x.getClass; x
|}
""".stripMargin

Expand All @@ -132,7 +132,7 @@ class InlineBytecodeTests extends DottyBytecodeTest {
diffInstructions(instructions1, instructions2))
}
}

*/
@Test def i4947 = {
val source = """class Foo {
| transparent inline def track[T](inline f: T): T = {
Expand Down
9 changes: 8 additions & 1 deletion library/src/scala/runtime/Scala3RunTime.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,16 @@ object Scala3RunTime:
*
* Extracted to minimize the bytecode size at call site.
*/
@deprecated("use Predef.nn instead", "3.2")
def nn[T](x: T | Null): x.type & T =
val isNull = x == null
if (isNull) throw new NullPointerException("tried to cast away nullability, but value is null")
if isNull then nnFail()
else x.asInstanceOf[x.type & T]

/** Called by the inline extension def `nn`.
*
* Extracted to minimize the bytecode size at call site.
*/
def nnFail(): Nothing =
throw new NullPointerException("tried to cast away nullability, but value is null")
end Scala3RunTime
3 changes: 2 additions & 1 deletion library/src/scala/runtime/stdLibPatches/Predef.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,8 @@ object Predef:
* }}}
*/
extension [T](x: T | Null) inline def nn: x.type & T =
scala.runtime.Scala3RunTime.nn(x)
if x.asInstanceOf[Any] == null then scala.runtime.Scala3RunTime.nnFail()
x.asInstanceOf[x.type & T]
Comment on lines +49 to +50
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you try

Suggested change
if x.asInstanceOf[Any] == null then scala.runtime.Scala3RunTime.nnFail()
x.asInstanceOf[x.type & T]
x.getClass() // throws NPE if x is null, no-op otherwise
x.asInstanceOf[x.type & T]

I've seen javac emit dummy calls to getClass() like this for the purpose of throwing NPEs. It might be even more efficient if the JVM recognizes the pattern.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That might lead to somewhat confusing error messages on recent JVMs where the NPE message tells you what operation caused the exception: https://openjdk.java.net/jeps/358 (would be interesting to see if modern javac still emits the dummy calls you saw)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, that's probably a good idea.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we find out

  • what error message gets emitted for a x.getClass call if x is null?
  • whether javac in later versions still emits the getClass or whether it does something else?

It would be good to do the same thing as javac since that's probably the smallest and fastest.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if the JVM does some better optimization for java.util.Objects.requireNonNull.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe requireNonNull is not the best we can do since it requires a useless cast after the call. We want to do the fastest possible non-null check (which should return Unit), followed by the cast to the target type.


extension (inline x: AnyRef | Null)
/** Enables an expression of type `T|Null`, where `T` is a subtype of `AnyRef`, to be checked for `null`
Expand Down