Skip to content

Conversation

@mbovel
Copy link
Member

@mbovel mbovel commented Dec 12, 2025

Attempt to move the ClassPath cache to an explicit GlobalCache to avoid global state, as suggested by @sjrd and @hamzaremmal.

We could add other kinds of caches there in the future, such as a file-contents cache as in #24650.

Given the wide variety of Driver implementations in the wild, this change is potentially risky: some may lose caching capabilities and become slower without noticing.

@mbovel
Copy link
Member Author

mbovel commented Dec 12, 2025

Setting the global cache in Driver.setup doesn't catch all context usages. There are direct, driver-less usages of ContextBase.initialContext, such as in DottyTest:

protected def initialCtx: FreshContext = {
val base = new ContextBase {}
val ctx = base.initialCtx.fresh

To be complete, we should probably set the cache in FreshContext.initial already, along with other store locations:

c._store = initialStore
.updated(settingsStateLoc, settingsGroup.defaultState)
.updated(notNullInfosLoc, Nil)
.updated(compilationUnitLoc, NoCompilationUnit)
.updated(profilerLoc, Profiler.NoOp)

But then, how to pass the cache instance down there? We don't even have access to settings there, I believe?

@mbovel
Copy link
Member Author

mbovel commented Dec 12, 2025

Example failure:

Test dotty.tools.repl.ShadowingBatchTests.directory failed: java.lang.NullPointerException:
Cannot invoke "dotty.tools.dotc.GlobalCache.getOrCreateClassPath(dotty.tools.io.AbstractFile,
scala.Function0, dotty.tools.dotc.core.Contexts$Context)" because the return value of
"dotty.tools.dotc.core.Contexts$Context.globalCache()" is null, took 0.516 sec

Error:      at dotty.tools.dotc.classpath.ZipAndJarFileLookupFactory.create(ZipAndJarFileLookupFactory.scala:28)
Error:      at dotty.tools.dotc.classpath.ZipAndJarFileLookupFactory.create$(ZipAndJarFileLookupFactory.scala:24)
...
Error:      at dotty.tools.dotc.core.Definitions.syntheticCoreClasses(Definitions.scala:2132)
Error:      at dotty.tools.dotc.core.Definitions.init(Definitions.scala:2148)
Error:      at dotty.tools.dotc.core.Contexts$ContextBase.initialize(Contexts.scala:941)
Error:      at dotty.tools.DottyTest.initialCtx(DottyTest.scala:31)
Error:      at dotty.tools.DottyTest.initialCtx$(DottyTest.scala:19)
...

https://github.com/scala/scala3/actions/runs/20161879826/job/57879475677?pr=24737#step:5:1318

*/
private class FileBasedCache[T]:
private case class Stamp(lastModified: FileTime, fileKey: Object)
private val cache = collection.mutable.Map.empty[java.nio.file.Path, (Stamp, T)]
Copy link
Member

Choose a reason for hiding this comment

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

if its no longer based on weak references, then can we used a concurrent map rather than synchronized?

Copy link
Member

Choose a reason for hiding this comment

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

or i guess you could want a per-key synchronization

Copy link
Member Author

@mbovel mbovel Dec 12, 2025

Choose a reason for hiding this comment

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

Note that it is a different cache than the one we discussed in #24650. This one already exists and caches ClassPath instances, not file contents. I just moved that code and haven't changed it.

There might be room for improvement, but it should be addressed separately. The questions I am trying to answer in this PR are just: can and should we make that global cache less global by explicitly passing instances to drivers.

But I agree it might be smarter to use a specialized concurrent map.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants