Fix #1692: Null out fields after use in lazy initialization #4289
Conversation
|
test performance please |
|
performance test scheduled: 1 job(s) in queue, 0 running. |
|
Performance test finished successfully: Visit http://dotty-bench.epfl.ch/4289/ to see the changes. Benchmarks is based on merging with master (2762567) |
| * - its type is nullable, or is an expression type (e.g. => Int) | ||
| * - is on used in a lazy val initializer | ||
| * - defined in the same class as the lazy val | ||
| * - TODO from Scalac? from a non-trait class |
There was a problem hiding this comment.
@retronym Scalac doesn't null out singly used private fields after use in lazy initialization if they are defined in a trait: https://github.com/scala/scala/blob/b3e380a08e4f8781f89412c4921f380a4b4758e9/src/compiler/scala/tools/nsc/transform/Mixin.scala#L381
What is the reason for this restriction?
allanrenucci
changed the title
| val name = "collectNullableFields" | ||
| } | ||
|
|
||
| /** Collect fields that can be null out after use in lazy initialization. |
There was a problem hiding this comment.
can be null -> can be nulled
| val name = "collectNullableFields" | ||
| } | ||
|
|
||
| /** Collect fields that can be null out after use in lazy initialization. |
| * fields that are only used within a lazy val initializer. This is not just an optimization, | ||
| * but is needed for correctness to prevent memory leaks. E.g. | ||
| * | ||
| * {{{ |
There was a problem hiding this comment.
Should a markdown code block instead
| ref(field).becomes(nullConst) | ||
| } | ||
| } | ||
|
|
||
| /** Create non-threadsafe lazy accessor equivalent to such code | ||
| * def methodSymbol() = { |
There was a problem hiding this comment.
Indentation is off... While you're at it, could you also add the missing code blocks in this file?
| val from = ctx.owner | ||
| val isNullable = | ||
| from.is(Lazy) && from.isField && // used in lazy field initializer | ||
| from.owner.eq(sym.owner) // lazy val and field in the same class |
There was a problem hiding this comment.
Does this cover cases like this:?
class TestByNameLazy(byNameMsg: => String) {
lazy val byLazyValMsg = {
def foo = byNameMsg
foo
}
}There was a problem hiding this comment.
No, I am being conservative here (so is Scalac).
In this case it is Ok to null out byNameMsg once byLazyValMsg is initialised. But there are cases where a reference to byNameMsg might leak out of the lazy val initialiser. For example:
trait Foo { def bar: String }
class TestByNameLazy(byNameMsg: => String) {
lazy val lazyFoo = {
def foo = byNameMsg
new Foo { def bar = foo }
}
lazyFoo.bar // not Ok to null out byNameMsg
}I haven't put too much thought into figuring out when it is safe to null out byNameMsg, but I believe requiring that the lazy val and the field are defined in the same class, and when using the field the owner is the lazy val is restrictive enough.
There was a problem hiding this comment.
Sounds good to me!
| case _ => | ||
| } | ||
|
|
||
| result.mapValues(_.toList).toMap |
There was a problem hiding this comment.
Maybe you could return a Map[Symbol, Seq[Symbol]] to avoid this conversion.
There was a problem hiding this comment.
I will change it to collection.Map[Symbol, List[Symbol]] to avoid the conversion from mutable map to immutable map. The conversion from ListBuffer to List is pretty much free since mapValues return a view on the collection.
There was a problem hiding this comment.
Actually I took a different approach. I return a mutable map so that LazyVals can clears entries that are not needed anymore
| @@ -73,6 +73,7 @@ class Compiler { | |||
| new LiftTry, // Put try expressions that might execute on non-empty stacks into their own methods | |||
| new HoistSuperArgs, // Hoist complex arguments of supercalls to enclosing scope | |||
| new ClassOf, // Expand `Predef.classOf` calls. | |||
| new CollectNullableFields, // Collect fields that can be null out after use in lazy initialization | |||
There was a problem hiding this comment.
Any reason to do this so early? I think the closer the collection is to the LazyVals phase, the better. It cannot be in the same group as LazyVals or the group before since that's Erasure, but it could be in the ErasedDecls group, no?
There was a problem hiding this comment.
I tried, but I think I need to run before Getters. Consider the following example:
class LazyNotNullable {
private val a = 'A'.toInt // not nullable type
lazy val l0 = a
}If I run after getter, I only see a as def a: => Int. But then, how can I differentiate between this a and the following:
class LazyNullable(a: => Int) {
lazy val l0 = a
}The first a is not nullable, the second is. Any suggestions?
There was a problem hiding this comment.
If you run after ElimByName then all vals with type => Int will have been replaced by vals with type () => Int, so you don't need to null anything with ExprType.
|
|
||
|
|
||
| override def prepareForUnit(tree: Tree)(implicit ctx: Context) = { | ||
| lazyValNullables = ctx.collectNullableFieldsPhase.asInstanceOf[CollectNullableFields].lazyValNullables |
There was a problem hiding this comment.
Ideally we should clear() this map once we're done with it, this can be done by overriding transformUnit.
| private[this] var nullability: IdentityHashMap[Symbol, FieldInfo] = _ | ||
|
|
||
| override def prepareForUnit(tree: Tree)(implicit ctx: Context) = { | ||
| nullability = new IdentityHashMap |
There was a problem hiding this comment.
This will do the wrong thing if you have more than one unit, by the time you get to the group where LazyVals is present, this method has been called for each unit, so all maps but the last one are forgotten.
|
test performance with #fast please |
|
performance test scheduled: 1 job(s) in queue, 1 running. |
|
Performance test finished successfully: Visit http://dotty-bench.epfl.ch/4289/ to see the changes. Benchmarks is based on merging with master (c28ea78) |
allanrenucci
force-pushed
the
fix-1692
branch
3 times, most recently
from
31fb8b6 to
968b7de
Compare
|
The optimisation triggers twice in the Dotty code base:
|
allanrenucci
force-pushed
the
fix-1692
branch
2 times, most recently
from
1df91db to
a56c5ae
Compare
| private[this] var lazyValNullables: IdentityHashMap[Symbol, mutable.ListBuffer[Symbol]] = _ | ||
| private def nullableFor(sym: Symbol)(implicit ctx: Context) = { | ||
| val nullables = lazyValNullables.remove(sym) | ||
| if (nullables == null || sym.is(Flags.Module)) Nil |
There was a problem hiding this comment.
If the Module check is needed, add a comment explaining why
| private[this] case class Nullable(by: Symbol) extends FieldInfo | ||
|
|
||
| /** Whether or not a field is nullable */ | ||
| private[this] var nullability: IdentityHashMap[Symbol, FieldInfo] = _ |
There was a problem hiding this comment.
You can use MutableSymbolMap instead.
| /** Running after `ElimByName` to see by names as nullable types. | ||
| * | ||
| * We don't necessary need to run after `Getters`, but the implementation | ||
| * could be simplified if we were to run before. |
There was a problem hiding this comment.
This comment isn't very clear, what simplifications?
Private fields that are only used during lazy val initialization can be assigned null once the lazy val is initialized. This is not just an optimization, but is needed for correctness to prevent memory leaks.
Fix #1692.
Private fields that are only used during lazy val initialization can be assigned null once the lazy val is initialized. This is not just an optimization, but is needed for correctness to prevent memory leaks.