Cache classloaders in Splicer #4235
Conversation
|
test performance please |
|
performance test scheduled: 1 job(s) in queue, 0 running. |
|
While inlining 1000 times |
|
Performance test finished successfully: Visit http://dotty-bench.epfl.ch/4235/ to see the changes. Benchmarks is based on merging with master (14187e2) |
|
test performance please |
|
performance test scheduled: 1 job(s) in queue, 0 running. |
| @@ -93,6 +93,8 @@ object Splicer { | |||
| } | |||
| } | |||
|
|
|||
| private val classLoaders = scala.collection.mutable.HashMap.empty[String, URLClassLoader] | |||
|
|
|||
There was a problem hiding this comment.
It means the cache is going to be shared by all compiler runs? I'm not sure about the following:
- concurrency
- memory leak
- setting changes between runs
I'm not familiar with these areas, could you explain?
There was a problem hiding this comment.
Concurrency should not be an issue. In the worst case, we create two different classloaders for the same classpath. Which is already happening now, hence the performance.
There is a potential memory leak as classloaders will be cached indefinitely. Maybe I could make it a sized cache that drops the oldest elements if it overflows. Or, maybe use a weak map.
Maybe I should invalidate the cache if the run has changed.
There was a problem hiding this comment.
I changed the Splicer to a class and instantiate one per run to avoid the issues above.
There was a problem hiding this comment.
It is not to share s.c.m.HashMap between threads without synchronization.
I'm thinking about using a timed cache in Scalac to deal with the leak problem (keep the loader around for X seconds after last use).
There was a problem hiding this comment.
The other issue with caching classloaders is having a good plan for invalidating them when JARs and directories on disk change.
There was a problem hiding this comment.
Now the cache is only valid during one phase of one run of the compiler. None of these should be an issue.
|
test performance please |
|
performance test scheduled: 3 job(s) in queue, 1 running. |
|
Performance test finished successfully: Visit http://dotty-bench.epfl.ch/4235/ to see the changes. Benchmarks is based on merging with master (14187e2) |
| @@ -21,9 +21,11 @@ import dotty.tools.dotc.util.Positions.Position | |||
| import scala.reflect.ClassTag | |||
|
|
|||
| /** Utility class to splice quoted expressions */ | |||
| object Splicer { | |||
| class Splicer { | |||
There was a problem hiding this comment.
It's weird to make the whole thing a class if the only bit of state is the classloader cache used in a single method, the cache could be passed as an argument instead
nicolasstucki
force-pushed
the
cache-classloader-in-splicer
branch
from
5cf42a5 to
0d1243a
Compare
|
test performance please |
|
performance test scheduled: 3 job(s) in queue, 1 running. |
| @@ -88,7 +88,8 @@ import dotty.tools.dotc.core.quoted._ | |||
| class ReifyQuotes extends MacroTransformWithImplicits with InfoTransformer { | |||
| import ast.tpd._ | |||
|
|
|||
| private lazy val splicer: Splicer = new Splicer | |||
| /** Classloader used for loading macros */ | |||
| private var macroClassLoader: java.lang.ClassLoader = _ | |||
There was a problem hiding this comment.
private[this] also usually we put the field and the getter next to each other, and we call the field myXXX and the getter XXX.
|
Performance test finished successfully: Visit http://dotty-bench.epfl.ch/4235/ to see the changes. Benchmarks is based on merging with master (1fdb94b) |
|
test performance please |
|
performance test scheduled: 6 job(s) in queue, 1 running. |
|
Performance test finished successfully: Visit http://dotty-bench.epfl.ch/4235/ to see the changes. Benchmarks is based on merging with master (1fdb94b) |
|
Performance test finished successfully: Visit http://dotty-bench.epfl.ch/4235/ to see the changes. Benchmarks is based on merging with master (7d01268) |
|
test performance please |
|
performance test scheduled: 112 job(s) in queue, 1 running. |
|
Performance test finished successfully: Visit http://dotty-bench.epfl.ch/4235/ to see the changes. Benchmarks is based on merging with master (4c8a2bd) |
No description provided.