-
Notifications
You must be signed in to change notification settings - Fork 56
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 19 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 |
---|---|---|
|
@@ -24,7 +24,11 @@ trait SjsonnetServerMain[T]{ | |
wd: os.Path): (Boolean, Option[T]) | ||
} | ||
|
||
object SjsonnetServerMain extends SjsonnetServerMain[collection.mutable.HashMap[(Path, String), Either[String, (Expr, FileScope)]]]{ | ||
// [Ask] SjsonnetServerMain has a valid main method (args0: Array[String]): Unit, | ||
// [warn] but sjsonnet.SjsonnetServerMain will not have an entry point on the JVM. | ||
// [warn] Reason: companion is a trait, which means no static forwarder can be generated. | ||
// [warn] object SjsonnetServerMain extends SjsonnetServerMain[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. What's with this comment? Is this an issue that should be addressed? 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. This got generated when I ran 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. Ah, now I also got the warning. I haven't seen it before. It's probably a real issue, but not caused by this PR. SjsonnetServerMain was already a trait with a companion object. |
||
object SjsonnetServerMain extends SjsonnetServerMain[DefaultParseCache]{ | ||
def main(args0: Array[String]): Unit = { | ||
// Disable SIGINT interrupt signal in the Mill server. | ||
// | ||
|
@@ -45,7 +49,7 @@ object SjsonnetServerMain extends SjsonnetServerMain[collection.mutable.HashMap[ | |
).run() | ||
} | ||
def main0(args: Array[String], | ||
stateCache: Option[collection.mutable.HashMap[(Path, String), Either[String, (Expr, FileScope)]]], | ||
stateCache: Option[DefaultParseCache], | ||
mainInteractive: Boolean, | ||
stdin: InputStream, | ||
stdout: PrintStream, | ||
|
@@ -55,7 +59,7 @@ object SjsonnetServerMain extends SjsonnetServerMain[collection.mutable.HashMap[ | |
wd: os.Path) = { | ||
|
||
val stateCache2 = stateCache.getOrElse{ | ||
val p = collection.mutable.HashMap[(Path, String), Either[String, (Expr, FileScope)]]() | ||
val p = new DefaultParseCache | ||
this.stateCache = Some(p) | ||
p | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,7 +6,7 @@ import scala.scalajs.js.annotation.{JSExport, JSExportTopLevel} | |
|
||
@JSExportTopLevel("SjsonnetMain") | ||
object SjsonnetMain { | ||
def createParseCache() = collection.mutable.HashMap[(Path, String), Either[String, (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. Can we deprecate this method? We have the default encapsulated nicely in 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. Yes, that makes sense. Updated |
||
@JSExport | ||
def interpret(text: String, | ||
extVars: js.Any, | ||
|
@@ -28,8 +28,8 @@ object SjsonnetMain { | |
def read(path: Path): Option[String] = | ||
Option(importLoader(path.asInstanceOf[JsVirtualPath].path)) | ||
}, | ||
preserveOrder, | ||
parseCache = createParseCache() | ||
parseCache = createParseCache(), | ||
preserveOrder | ||
) | ||
interp.interpret0(text, JsVirtualPath("(memory)"), ujson.WebJson.Builder) match{ | ||
case Left(msg) => throw new js.JavaScriptException(msg) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,7 +8,7 @@ import scala.util.Try | |
import scala.util.control.NonFatal | ||
|
||
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 +33,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 +44,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 +127,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] = { | ||
|
@@ -182,10 +182,10 @@ object SjsonnetMain { | |
} | ||
case None => resolveImport(config.jpaths.map(os.Path(_, wd)).map(OsPath(_)), allowedInputs) | ||
}, | ||
parseCache, | ||
preserveOrder = config.preserveOrder.value, | ||
strict = config.strict.value, | ||
storePos = if (config.yamlDebug.value) currentPos = _ else null, | ||
parseCache | ||
storePos = if (config.yamlDebug.value) currentPos = _ else null | ||
) | ||
|
||
(config.multi, config.yamlStream.value) match { | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,15 @@ | ||
package sjsonnet | ||
|
||
// 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) | ||
} | ||
} |
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.
Let's leave this at 0.4.1, we're going to cut a 0.4.1 release from it anyway. (We could update to 0.4.2-SNAPSHOT afterwards, but it's probably not worth the effort)
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.
Okay updated