Fix caches of info that depends on inherited classes #2425

odersky · 2017-05-13T13:05:55Z

Change from the ground up the way we treat inherited information. We get rid of:

superId structures,
fingerprints
Frozen and FullyCompleted flags,
restrictions on when we can enter new symbols in scopes
most manual cache invalidation

We treat systematically the problem of cache dependencies by tracking reverse dependencies with weak hashmaps. We need cooperation from phases to declare what kind of inherited info they can change.

There are two new kinds of caches:

MemberNames contains various kinds of member name sets indexed by name filter
BaseData contains baseClasses (i.e. base class linearization), and BaseClassSet, which
is a sorted-array-backed set for fast subclass checking. BaseClassSets replace the
superId maps we had before.

The PR fixes several problems with inherited info. This manifested itself in the past in a fingerprint problem when function specialization is turned on. But fingerprints are just where the problem arises first; it is also possible to hit problems with the other caches.

Also introduce dummy onBehalfs as sentinels to avoid the need for `null` or `Option`.

After erasure all basedata computations were marked as provisional because Object has empty parents then. This caused slowdowns of up to 100% (~100% was observed for stdlib).

- transformers caches as long as they are valid in new phase - avoid state in invalid dummy caches, so that they don't need to be put in contexts - refactor caches into traits and implementation classes

The lgoxi is much nicer without it.

No longer needed because we have proper cache invalidation now

It's wow subject to the same invalidation as base data caches (except that there's no need for recursive invalidation of caches in subclasses)

(reverted from commit c1254ed) Last data over whole legacy test suite (running time: 337s): BaseData cache fills per phase: 0, 670675, 0, 0, 2648, 0, 12, 0, 2, 548, 1153, 1189, 190, 115, 0, 0, 0, 0, 28, 68, 0, 0, 129, 267, 0, 0, 0, 0, 0, 4, 0, 38, 0, 3, 13, 1, 6857, 809, 167147, 59961, 671, 246, 5417, 169, 114, 615, 17, 0, 2, 0, 1753, 0, 27, 0, 0, 8032, 26338, 0, 0, 736, 0, 0, 0, 4851, 69224, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0 Cache invalidations: 80

smarter · 2017-05-14T18:36:22Z

compiler/src/dotty/tools/dotc/core/SymDenotations.scala

+
+    final def isValid(implicit ctx: Context): Boolean = valid && isValidAt(ctx.phase)
+
+    def invalidate(): Unit =


Missing an (implicit ctx: Context)

smarter · 2017-05-14T18:38:41Z

Nice that we can unify logic for all these caches! Did you measure any performance difference compared to master?

DarkDimius · 2017-05-15T08:49:33Z

@smarter, I'm reviewing this PR now and there are things that may affect performance. I'd propose to benchmark after review, but before merging.

DarkDimius

A very nice systematization!

The high level comment is that changesMembers is not checked on all the paths. I guess it is because checking it is very slow.
I'd suggest to add such checks guarder by a Config flag, as it may save us a lot of time in case we discover subtle bugs.

Otherwise(and modulo @smarter's comment) looks very good to me.

DarkDimius · 2017-05-15T08:39:19Z

compiler/src/dotty/tools/dotc/core/Phases.scala

@@ -331,6 +339,8 @@ object Phases {
    final def refChecked = myRefChecked     // Phase is after RefChecks
    final def symbolicRefs = mySymbolicRefs // Phase is after ResolveSuper, newly generated TermRefs should be symbolic
    final def labelsReordered = myLabelsReordered // Phase is after LabelDefs, labels are flattened and owner chains don't mirror this
+    final def membersGroup = myMembersGroup // group id for phases where all symbols have the same non-private members


Either the doc & implementation are out of sync~, ~~or this is a bug~~.

The implementation counts how many times where parents changed, it does not use the actual phase-ids.
I've had a look through usages, this value seems to only be used for equality comparisons, so the "number of times members could have changed" would behave correctly, but I'd still propose to make it return a PhaseID to ease understanding.

Same with parentsGroup.

Good point. I'll change to phase ids, and use a more descriptive name.

DarkDimius · 2017-05-15T08:41:21Z

compiler/src/dotty/tools/dotc/core/SymDenotations.scala

      val info1 = if (info != null) info else this.info
+      if (ctx.isAfterTyper && changedClassParents(info, info1, completersMatter = false))
+        assert(ctx.phase.changesParents, i"undeclared parent change at ${ctx.phase} for $this, was: $info, now: $info1")


maybe add a check that also checks correctness of changesMembers assumption here?
It may be slow, so I'd propose to guard it with config flag.

The check is in enteredAfter. I think it would make little sense checking in info_= because typically we set the info with a clone of the previous scope and then add symbols later using enteredAfter.

DarkDimius · 2017-05-15T08:42:13Z

compiler/src/dotty/tools/dotc/core/SymDenotations.scala

@@ -1209,6 +1230,68 @@ object SymDenotations {

    import util.LRUCache

+    // ----- caches -------------------------------------------------------
+
+    private[this] var myTypeParams: List[TypeSymbol] = null


rename to myTypeParamsCache for consistency?

DarkDimius · 2017-05-15T08:43:27Z

compiler/src/dotty/tools/dotc/core/SymDenotations.scala

+    private var memberNamesCache: MemberNames = MemberNames.None
+
+    private def memberCache(implicit ctx: Context): LRUCache[Name, PreDenotation] = {
+      if (myMemberCachePeriod != ctx.period) {


I'd propose to check for inclusion instead of equality: !myMemberCachePeriod.contains(ctx.period)

The reason is that mini-phase driver runs with ctx.period set to multiple phases.

But myMemberCachePeriod is always set to a single phase period, right?

DarkDimius · 2017-05-15T08:43:46Z

compiler/src/dotty/tools/dotc/core/SymDenotations.scala

+    }
+
+    private def baseTypeRefCache(implicit ctx: Context): BaseTypeRefMap = {
+      if (myBaseTypeRefCachePeriod != ctx.period &&


same here, contains instead of equality.

DarkDimius · 2017-05-15T08:51:51Z

compiler/src/dotty/tools/dotc/core/SymDenotations.scala

-      super.info_=(tp)
+      if (changedClassParents(infoOrCompleter, tp, completersMatter = true))
+        invalidateBaseDataCache()
+      invalidateMemberNamesCache()


I'd propose to add a check similar to changedClassParents for members, which is guarder by a Config flag.

member names are relatively infrequently needed, so I think we lose little to invalidate them more aggressively than strictly needed. Also, names will usually be entered later using enteredAfter.

DarkDimius · 2017-05-15T08:53:23Z

compiler/src/dotty/tools/dotc/core/SymDenotations.scala

-        myMemberFingerPrint.include(sym.name)
-      if (myMemberCache != null)
-        myMemberCache invalidate sym.name
+      if (myMemberCache != null) myMemberCache.invalidate(sym.name)


DarkDimius · 2017-05-15T08:53:43Z

compiler/src/dotty/tools/dotc/core/SymDenotations.scala

      unforcedDecls.openForMutations.replace(prev, replacement)
-      if (myMemberCache != null)
-        myMemberCache invalidate replacement.name
+      if (myMemberCache != null) myMemberCache.invalidate(replacement.name)


DarkDimius · 2017-05-15T08:54:43Z

compiler/src/dotty/tools/dotc/core/SymDenotations.scala

      info.decls.openForMutations.unlink(sym)
-      myMemberFingerPrint = FingerPrint.unknown
-      if (myMemberCache != null) myMemberCache invalidate sym.name
+      if (myMemberCache != null) myMemberCache.invalidate(sym.name)


Is caching private members worth it?

DarkDimius · 2017-05-15T09:06:01Z

compiler/src/dotty/tools/dotc/core/SymDenotations.scala

+    }
+
+    private def invalidateBaseDataCache() = {
+      baseDataCache.invalidate()


why do you need both call to invalidate and assignment to an empty cache?

In copyCaches you copy over the refernce from previous cache. It means that invalidating a future cache will also invalidate an old cache, which is still correct.

Same in other invalidation methods.

Good point. It was originally to save space, but I don't think that matters in practice.

In fact no. It does matter. IsValid at only checks that the creation period is compatible with the current one. Invalidation does not affect the creation period. So we need another way to signal that the cache is nowhere valid. That's what the assignment does.

odersky added 7 commits May 12, 2017 13:39

Use proper cache for memberNames

bf9123d

Improve caching of baseClasses and superClassBits

6d525f1

Make onBehalf implicit parameters

bf84d5a

Also introduce dummy onBehalfs as sentinels to avoid the need for `null` or `Option`.

Get rid of some old code that's no longer used

e9b98cf

Void basedata computation inefficiency

ff7bdd5

After erasure all basedata computations were marked as provisional because Object has empty parents then. This caused slowdowns of up to 100% (~100% was observed for stdlib).

Instrumentation to get basedata computation counts

c1254ed

Have phases declare whether they can change members or parents

e72c052

odersky added the stat:wip label May 13, 2017

odersky added 12 commits May 13, 2017 17:54

Refactorings to caches

d2b3fff

- transformers caches as long as they are valid in new phase - avoid state in invalid dummy caches, so that they don't need to be put in contexts - refactor caches into traits and implementation classes

Don't use null for inherited caches

7a2f158

The lgoxi is much nicer without it.

Invalidate less on info_=

12b91f9

Remove unnecessary cache invalidation

7afda82

Remove FingerPrints

ddbf8f7

Get rid of Frozen flag

e86e314

No longer needed because we have proper cache invalidation now

Get rid of FullyCompleted flag

6c13d54

Mive fullNameCache to caches section

91c9013

Document caches for inherited info

d648fd1

Get rid of superIds

44b1a9b

Fix handling of baseTypeRef caches.

51aebce

It's wow subject to the same invalidation as base data caches (except that there's no need for recursive invalidation of caches in subclasses)

Polish docs

3f2ff0c

odersky changed the title ~~WIP: Fix inherited caches~~ Fix caches of info that depends on inherited classes May 14, 2017

odersky requested a review from DarkDimius May 14, 2017 09:29

smarter reviewed May 14, 2017

View reviewed changes

DarkDimius approved these changes May 15, 2017

View reviewed changes

odersky added 2 commits May 15, 2017 15:58

Fix reviewers comments

04bc040

Address reviewers comments

effaca1

odersky removed the stat:wip label May 16, 2017

nicolasstucki mentioned this pull request May 17, 2017

Fingerprints broken when adding base classes in later phases #2417

Closed

felixmulder merged commit 2b34272 into scala:master May 29, 2017

allanrenucci deleted the fix-inherited-caches branch December 14, 2017 19:20


		final def isValid(implicit ctx: Context): Boolean = valid && isValidAt(ctx.phase)

		def invalidate(): Unit =

Fix caches of info that depends on inherited classes #2425

Fix caches of info that depends on inherited classes #2425

Uh oh!

Conversation

odersky commented May 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

smarter commented May 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DarkDimius commented May 15, 2017

Uh oh!

DarkDimius left a comment

Choose a reason for hiding this comment

Uh oh!

DarkDimius May 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

odersky commented May 13, 2017 •

edited

Loading

smarter commented May 14, 2017 •

edited

Loading

DarkDimius May 15, 2017 •

edited

Loading