Optimize Infos for caching in InfoLoader #4981
Conversation
It was a useful abstraction when we were using Traversers to build entire class infos. However, at this point it's mostly boilerplate. Changeing this, will help simplify upcoming changes.
gzm0
force-pushed
the
less-mutable
branch
2 times, most recently
from
63444a1
to
adac530
Compare
| @@ -1517,19 +1516,17 @@ private class AnalyzerRun(config: CommonPhaseConfig, initial: Boolean, | |||
| } | |||
|
|
|||
| private def makeSyntheticMethodInfo( | |||
| methodName: MethodName, | |||
| namespace: MemberNamespace, | |||
| methodsCalled: List[(ClassName, MethodName)] = Nil, | |||
| methodsCalledStatically: List[(ClassName, NamespacedMethodName)] = Nil, | |||
| instantiatedClasses: List[ClassName] = Nil | |||
There was a problem hiding this comment.
Reading this, I realize instantiatedClasses is dead code. Take the opportunity to remove it?
| prevJSCtorInfo = classDef.jsConstructor.map { ctor => | ||
| prevJSCtorInfo | ||
| .filter(_.version.sameVersion(ctor.version)) | ||
| .getOrElse(Infos.generateJSConstructorInfo(ctor)) | ||
| } |
There was a problem hiding this comment.
Nit: extract into genJSConstructorInfo(classDef.jsConstructor) to be consistent with the other two?
| val jsMethodProps = { | ||
| classDef.jsConstructor.map(jsConstructorInfoCache.getInfo(_)).toList ::: | ||
| exportedMembersInfoCaches.getInfos(classDef.jsMethodProps) | ||
| prevMethodInfos = genMethodInfos(classDef.methods) |
There was a problem hiding this comment.
It's a bit confusing that genMethodInfos uses the previous state of prevMethodInfos right before we reassign it. It doesn't help that there are two such methods that each access state that happens to be reassigned after them, but not the other state. It's not a big deal but it is a big hard to wrap my head around why it's safe.
It seems prevMethodInfos is in fact the only piece of state used by genMethodInfos. Did you consider explicitly passing prevMethodInfos as an argument to genMethodInfos and put the latter in a scope where it cannot access any state?
Concretely, amend this line to
| prevMethodInfos = genMethodInfos(classDef.methods) | |
| prevMethodInfos = genMethodInfos(classDef.methods, prevMethodInfos) |
and move genMethodInfos outside of private class ClassInfoCache.
The same applies to genJSMethodPropDefInfos and possibly genJSConstructorInfo.
There was a problem hiding this comment.
yes, I agree. I didn't like this as well when I was writing it. Just didn't really follow through on the feeling.
I changed it.
gzm0
force-pushed
the
less-mutable
branch
2 times, most recently
from
a0ebe2b
to
b3549f8
Compare
This allows us to use the previously calculated infos themselves as the cached values, reducing memory consumption. Further, we reduce memory consumption by using immutable maps: Because a lot of the maps are empty, we do not spend unecessary memory on empty mutable maps. This reduces the retained size on the test suite for the infos as follows: | Component | Before [MB] | After [MB] | |------------|------------:|-----------:| | BaseLinker | 20 | 15 | | Refiner | 17 | 13 | As a nice side-effect, this allows us to simplify the InfoLoader. The simplification mainly stems from the insight, that we do not need active cache used tracking; control flow is sufficient. Execution times are unaffected, in fact, the incremental case might even be a bit faster (non-significant though).
This allows us to use the previously calculated infos themselves as
the cached values, reducing memory consumption.
Further, we reduce memory consumption by using immutable maps: Because
a lot of the maps are empty, we do not spend unecessary memory on
empty mutable maps.
This reduces the retained size on the test suite for the infos as
follows:
As a nice side-effect, this allows us to simplify the InfoLoader. The
simplification mainly stems from the insight, that we do not need
active cache used tracking; control flow is sufficient.
Execution times are unaffected, in fact, the incremental case might
even be a bit faster (non-significant though).