Use a simple cache based on futures to avoid redownloading on concurrent usage #1384
Conversation
| } | ||
| } match { | ||
| case Right(fut) => | ||
| /*fut.value match { |
There was a problem hiding this comment.
I left the printlns in for now but can remove them if required.
There was a problem hiding this comment.
I normally don't leave printlns in the code but I don't have a strong preference against them. Let's remove it here for consistency with the rest of the codebase
|
Though CI failed (test for Scala 2.11, and scalafmtCheck), it looks nice at a glance. |
There was a problem hiding this comment.
Thank you for this contribution! This is a good idea. @jrudolph Only blocking comment is to remove the comments, we can keep ReentrantCache if you think that is a better solution than synchronized maps.
To fix the CI
./scalafmt --diff
Also, to fix the compile error on 2.11 we can add -Xexperimental to build or remove the following SAM type
foreach is not a member of org.scalafmt.dynamic.ScalafmtDynamic.FormatEval[org.scalafmt.dynamic.ScalafmtReflect]
[error] formatCache.clear().foreach(_.foreach(_.foreach(_.classLoader.close()))(ExecutionContext.global))
[error] ^
[error] /home/travis/build/scalameta/scalafmt/scalafmt-dynamic/src/main/scala/org/scalafmt/dynamic/ScalafmtDynamic.scala:109:50: value exists is not a member of org.scalafmt.dynamic.ScalafmtDynamic.FormatEval[(org.scalafmt.dynamic.ScalafmtReflectConfig, java.nio.file.attribute.FileTime)]
[error] configsCache.getOrAddToCache(configPath, _.exists(_._2.compareTo(currentTimestamp) != 0)) { () =>
[error] ^
| } | ||
| } match { | ||
| case Right(fut) => | ||
| /*fut.value match { |
There was a problem hiding this comment.
I normally don't leave printlns in the code but I don't have a strong preference against them. Let's remove it here for consistency with the rest of the codebase
| import scala.concurrent.duration._ | ||
| import scala.util.Try | ||
|
|
||
| class ReentrantCache[K, V] { |
There was a problem hiding this comment.
Can we add a simple explanation of this data structure? For example "This is like a map except ..."
There was a problem hiding this comment.
Could we use Collections.synchronizedMap(...) and computeIfAbsent instead of ReentrantCache? To limit synchronization we could have another HashMap cache in front of that where we first try with get()
There was a problem hiding this comment.
I found that ReentrantCache is a helpful abstraction of the series of operations (check the result of download and then cache or not)) 👍
Therefore I think it isn't necessarily required to replacing this data structure with built-in data structures (synchronized or concurrent HashMap) as long as the performance doesn't matter.
It would be better to replace this with synchronizedMap or something while keeping the ScalafmtDynamic.scala simple, but I don't have any ideas now...
| case None => | ||
| //println(s"Previous calculation for $key still ongoing") | ||
| }*/ | ||
| val result = Await.result(fut, 1.minute) // get or wait for other thread to finish download |
There was a problem hiding this comment.
Is this a 1 minute timeout limit to complete the download? If so, I think we need to increase the timeout since we can't expect everybody to have the same internet connection speed. I would go with a large value like 10 minutes, users see the download progress in the console output and can cancel the process with ctrl-c.
tanishiking
force-pushed
the
fix-redownloading
branch
from
b78d7f1 to
ffee006
Compare
tanishiking
force-pushed
the
fix-redownloading
branch
from
ffee006 to
635e2b1
Compare
There was a problem hiding this comment.
@tanishiking LGTM 👍 thank you for looking into this. I'm still concerned about the 1 minute timeout for downloads since people have varying internet connections, I think it might be better to use Collections.synchronizedMap with no timeout limit.
|
Thanks :)
That's right... I'll merge this PR after modifying the timeout to 10 minutes for now. (Maybe we want to refactor it to use |
No description provided.