Skip to content

Optimize basetype caches #9608

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 3 commits into from
Aug 21, 2020
Merged

Optimize basetype caches #9608

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
250ce6f
Unconditionally copy non-package class denotations in erasure
odersky Jul 25, 2020
06e82e9
Rename mayChange -> infoMayChange
odersky Jul 25, 2020
6729e1c
Invalidate base type caches only if runId changes
odersky Aug 20, 2020
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/src/dotty/tools/dotc/core/DenotTransformers.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,7 @@ object DenotTransformers {

def transform(ref: SingleDenotation)(using Context): SingleDenotation = {
val sym = ref.symbol
if (sym.exists && !mayChange(sym)) ref
if (sym.exists && !infoMayChange(sym)) ref
else {
val info1 = transformInfo(ref.info, ref.symbol)
if (info1 eq ref.info) ref
Expand All @@ -50,11 +50,11 @@ object DenotTransformers {
}
}

/** Denotations with a symbol where `mayChange` is false are guaranteed to be
/** Denotations with a symbol where `infoMayChange` is false are guaranteed to be
* unaffected by this transform, so `transformInfo` need not be run. This
* can save time, and more importantly, can help avoid forcing symbol completers.
*/
protected def mayChange(sym: Symbol)(using Context): Boolean = true
protected def infoMayChange(sym: Symbol)(using Context): Boolean = true
}

/** A transformer that only transforms SymDenotations.
Expand Down
6 changes: 5 additions & 1 deletion compiler/src/dotty/tools/dotc/core/SymDenotations.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2641,7 +2641,11 @@ object SymDenotations {
private var locked = false
private var provisional = false

final def isValid(using Context): Boolean = valid && isValidAt(ctx.phase)
final def isValid(using Context): Boolean =
valid && createdAt.runId == ctx.runId
// Note: We rely on the fact that whenever base types of classes change,
// the affected classes will get new denotations with new basedata caches.
// So basedata caches can become invalid only if the run changes.
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not familiar with code in this area. I'm wondering will the cache be always valid for classes defined in libraries, regardless of runs?

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, but libraries might be redefined in source in the next run, so you need to re-check.


def invalidate(): Unit =
if (valid && !locked) {
Expand Down
2 changes: 1 addition & 1 deletion compiler/src/dotty/tools/dotc/transform/ElimByName.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ class ElimByName extends TransformByNameApply with InfoTransformer {
case _ => tp
}

override def mayChange(sym: Symbol)(using Context): Boolean = sym.isTerm && exprBecomesFunction(sym)
override def infoMayChange(sym: Symbol)(using Context): Boolean = sym.isTerm && exprBecomesFunction(sym)
}

object ElimByName {
Expand Down
2 changes: 1 addition & 1 deletion compiler/src/dotty/tools/dotc/transform/ElimRepeated.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ class ElimRepeated extends MiniPhase with InfoTransformer { thisPhase =>
case ref1 =>
ref1

override def mayChange(sym: Symbol)(using Context): Boolean = sym.is(Method)
override def infoMayChange(sym: Symbol)(using Context): Boolean = sym.is(Method)

private def overridesJava(sym: Symbol)(using Context) = sym.allOverriddenSymbols.exists(_.is(JavaDefined))

Expand Down
15 changes: 8 additions & 7 deletions compiler/src/dotty/tools/dotc/transform/Erasure.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,16 +90,17 @@ class Erasure extends Phase with DenotTransformer {
newFlags = newFlags &~ Flags.Inline
newAnnotations = newAnnotations.filterConserve(!_.isInstanceOf[BodyAnnotation])
// TODO: define derivedSymDenotation?
if (oldSymbol eq newSymbol)
&& (oldOwner eq newOwner)
&& (oldName eq newName)
&& (oldInfo eq newInfo)
&& (oldFlags == newFlags)
&& (oldAnnotations eq newAnnotations)
if ref.is(Flags.PackageClass)
|| !ref.isClass // non-package classes are always copied since their base types change
&& (oldSymbol eq newSymbol)
&& (oldOwner eq newOwner)
&& (oldName eq newName)
&& (oldInfo eq newInfo)
&& (oldFlags == newFlags)
&& (oldAnnotations eq newAnnotations)
then
ref
else
assert(!ref.is(Flags.PackageClass), s"trans $ref @ ${ctx.phase} oldOwner = $oldOwner, newOwner = $newOwner, oldInfo = $oldInfo, newInfo = $newInfo ${oldOwner eq newOwner} ${oldInfo eq newInfo}")
ref.copySymDenotation(
symbol = newSymbol,
owner = newOwner,
Expand Down
2 changes: 1 addition & 1 deletion compiler/src/dotty/tools/dotc/transform/ExplicitOuter.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,7 @@ class ExplicitOuter extends MiniPhase with InfoTransformer { thisPhase =>
tp
}

override def mayChange(sym: Symbol)(using Context): Boolean = sym.isClass && !sym.is(JavaDefined)
override def infoMayChange(sym: Symbol)(using Context): Boolean = sym.isClass && !sym.is(JavaDefined)

/** First, add outer accessors if a class does not have them yet and it references an outer this.
* If the class has outer accessors, implement them.
Expand Down
2 changes: 1 addition & 1 deletion compiler/src/dotty/tools/dotc/transform/FirstTransform.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@ class FirstTransform extends MiniPhase with InfoTransformer { thisPhase =>
tp
}

override protected def mayChange(sym: Symbol)(using Context): Boolean = sym.isClass
override protected def infoMayChange(sym: Symbol)(using Context): Boolean = sym.isClass

override def checkPostCondition(tree: Tree)(using Context): Unit =
tree match {
Expand Down