-
Notifications
You must be signed in to change notification settings - Fork 1.1k
Move the ClassPath cache to an explicit GlobalCache
#24737
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.
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
base: main
Are you sure you want to change the base?
Conversation
|
Setting the global cache in scala3/compiler/test/dotty/tools/DottyTest.scala Lines 25 to 27 in ba45875
To be complete, we should probably set the cache in scala3/compiler/src/dotty/tools/dotc/core/Contexts.scala Lines 773 to 777 in ba45875
But then, how to pass the cache instance down there? We don't even have access to settings there, I believe? |
|
Example failure: 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)] |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
Attempt to move the
ClassPathcache to an explicitGlobalCacheto 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
Driverimplementations in the wild, this change is potentially risky: some may lose caching capabilities and become slower without noticing.