-
Notifications
You must be signed in to change notification settings - Fork 55
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
[PLAT-33341] Add Caffeine interface to limit Sjsonnet worker cache size #128
Changes from 11 commits
5f1c4db
0c975b3
b260882
ee1766a
6db24c5
dd9323e
8288a59
d3f8048
cd31540
5b6cb8c
9399365
53cc9b6
9d210c1
13a4385
9474382
354b717
45a7998
8d0c663
3031867
278e37a
d462de0
cad3c82
842d937
2db27b7
558961a
30ad578
be733ed
c794cc9
78cb4c0
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,8 +7,22 @@ import java.nio.file.NoSuchFileException | |
import scala.util.Try | ||
import scala.util.control.NonFatal | ||
|
||
// Trait extended by JsonnetWorker (in universe) so that it can pass the cache based on Caffeine to main0 here | ||
trait ParseCache { | ||
def getOrElseUpdate(key: (Path, String), defaultValue: => Either[Error, (Expr, FileScope)]): Either[Error, (Expr, FileScope)] | ||
} | ||
|
||
// A default implementation based on a mutable HashMap. This implementation is not thread-safe. | ||
class DefaultParseCache extends ParseCache { | ||
val cache = new collection.mutable.HashMap[(Path, String), Either[Error, (Expr, FileScope)]]() | ||
|
||
override def getOrElseUpdate(key: (Path, String), defaultValue: => Either[Error, (Expr, FileScope)]): Either[Error, (Expr, FileScope)] = { | ||
cache.getOrElseUpdate(key, defaultValue) | ||
} | ||
} | ||
|
||
object SjsonnetMain { | ||
def createParseCache() = collection.mutable.HashMap[(Path, String), Either[Error, (Expr, FileScope)]]() | ||
def createParseCache() = new DefaultParseCache | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same here |
||
|
||
def resolveImport(searchRoots0: Seq[Path], allowedInputs: Option[Set[os.Path]] = None) = new Importer { | ||
def resolve(docBase: Path, importName: String): Option[Path] = | ||
|
@@ -33,7 +47,7 @@ object SjsonnetMain { | |
case Array(s, _*) if s == "-i" || s == "--interactive" => args.tail | ||
case _ => args | ||
}, | ||
collection.mutable.HashMap.empty, | ||
new DefaultParseCache, | ||
System.in, | ||
System.out, | ||
System.err, | ||
|
@@ -44,7 +58,7 @@ object SjsonnetMain { | |
} | ||
|
||
def main0(args: Array[String], | ||
parseCache: collection.mutable.HashMap[(Path, String), Either[Error, (Expr, FileScope)]], | ||
parseCache: ParseCache, | ||
stdin: InputStream, | ||
stdout: PrintStream, | ||
stderr: PrintStream, | ||
|
@@ -127,7 +141,7 @@ object SjsonnetMain { | |
|
||
def mainConfigured(file: String, | ||
config: Config, | ||
parseCache: collection.mutable.HashMap[(Path, String), Either[Error, (Expr, FileScope)]], | ||
parseCache: ParseCache, | ||
wd: os.Path, | ||
allowedInputs: Option[Set[os.Path]] = None, | ||
importer: Option[(Path, String) => Option[os.Path]] = None): Either[String, String] = { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -43,8 +43,8 @@ class CachedImporter(parent: Importer) extends Importer { | |
|
||
class CachedResolver( | ||
parentImporter: Importer, | ||
val parseCache: mutable.HashMap[(Path, String), Either[Error, (Expr, FileScope)]] = new mutable.HashMap | ||
) extends CachedImporter(parentImporter) { | ||
val parseCache: ParseCache = new DefaultParseCache | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Let's remove this default value so we can make sure we pass the right parse cache from all code paths. This means you'll have to find all places where this method is called and decide if it should pass a fresh |
||
) extends CachedImporter(parentImporter) { | ||
|
||
def parse(path: Path, txt: String)(implicit ev: EvalErrorScope): Either[Error, (Expr, FileScope)] = { | ||
parseCache.getOrElseUpdate((path, txt), { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,8 +18,8 @@ class Interpreter(extVars: Map[String, ujson.Value], | |
preserveOrder: Boolean = false, | ||
strict: Boolean = false, | ||
storePos: Position => Unit = null, | ||
val parseCache: mutable.HashMap[(Path, String), Either[Error, (Expr, FileScope)]] = new mutable.HashMap, | ||
) { self => | ||
val parseCache: ParseCache = new DefaultParseCache | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Same here, let's not have a default argument for the parse cache |
||
) { self => | ||
|
||
val resolver = new CachedResolver(importer, parseCache) { | ||
override def process(expr: Expr, fs: FileScope): Either[Error, (Expr, FileScope)] = | ||
|
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.
Can we deprecate this method? We have the default encapsulated nicely in
new DefaultParseCache
now and it is used directly in several places, so this method seems superfluous.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.
Yes, that makes sense. Updated