Remove various caches from JvmDependenciesIndexImpl
#2366
Conversation
|
|
||
| // root "Cache" object corresponds to DefaultPackage which exists in every root. Roots with non-default fqname are also listed here but | ||
| // they will be ignored on requests with invalid fqname prefix. | ||
| private val rootCache: Cache by lazy { |
There was a problem hiding this comment.
Main cache №1, the one we've been having problems with. It's also nested.
Basically maps packages to an index inside roots (separate field at the top) to reduce lookup time (no need to iterate all roots, just the roots of requested packages). Making this implementation (as is) thread safe could be problematic, more info in the PR description
|
|
||
| // holds the request and the result last time we searched for class | ||
| // helps improve several scenarios, LazyJavaResolverContext.findClassInJava being the most important | ||
| private var lastClassSearch: Pair<FindClassRequest, SearchResult>? = null |
There was a problem hiding this comment.
Cache №2. There are cache hits, actually more than I thought, so this could be useful. Could be made thread safe with volatile without much problem. No reports of it misbehaving, although there could've been invisible concurrency issues (since there's no synchronization)
| private val packageCache: Array<out MutableMap<String, VirtualFile?>> by lazy { | ||
| Array(roots.size) { THashMap<String, VirtualFile?>() } | ||
| } |
There was a problem hiding this comment.
Cache №3. Already synchronized on, so there shouldn't have been any problems with it. Seems to be the most important one as it saves most of the heavy lifting (stores the result of findChildPackage). I think this can be safely reverted
|
Speaking about original motivation: this index was copied to fix #1599, there was a problem with the Trove library |
IgnatBeresnev
force-pushed
the
jvm-dep-index-caches
branch
from
13157bb
to
d9a268f
Compare
java.lang.IllegalStateException: Recursive update is thrown
|
Stale. Will re-open if this is needed for tests again. |
Removed various caches and did some
dokkaHtmlMultiModulebenchmarking on kotlinx-coroutines.I benchmarked 4 times: baseline (current master) + once for each removed cache (
#1,#2and#3, will outline them by comments) to see the impact of each removed cache.The results (html link) show that there's no significant build time degradation, but the benchmarks are not as ideal as they could be:
kotlinx-coroutineswasn't the best project to benchmark on -- it doesn't have that many dependencies and roots, so the cache is relatively small in size. I wonder if the results will be any different on a large scale enterprise projectAlternatively, it's possible to add some basic thread safety to the current solution by synchronizing on individual
Cacheobjects when working with them, but because there's a section where two caches need to be locked consecutively, I can come up with a hypothetical concurrent scenario in which a deadlock happens (whether it could actually happen is a good question, need more time to investigate).P.S., turns out,
JvmDependenciesIndexImplwas copy-pasted from the kotlin repo (it also has caches), which makes it a little scary to remove (hopefully it was introduced to kotlin based on some testing). I was unable to find the PR or any attached issue to introducing the caches, so not sure what the motivation was behind it, and its primary contributor does not work at JB anymore, so I can't ask directly.