-
Notifications
You must be signed in to change notification settings - Fork 54
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
Static Object Optimizations #197
Conversation
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.
What's the performance impact of these changes? Are we trading memory for speed?
// This is a dummy benchmark to see how much memory is used by the interpreter. | ||
// You're meant to execute it, and once it prints "sleeping" you can attach yourkit and take a heap | ||
// dump. Because we store the cache, the parsed objects will have strong references - and thus will | ||
// be in the heap dump. |
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.
Could we turn this into a simple command line program that does a gc before and after parsing and then prints the memory usage diff to the console so you can easily retest this for future changes without having to attach a profiler?
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.
Done and updated readme.
// HashMap to deduplicate strings. | ||
private[this] val strings = new mutable.HashMap[String, String] | ||
|
||
private[this] val fieldSet = new mutable.HashMap[Val.StaticObjectFieldSet, java.util.LinkedHashMap[String, java.lang.Boolean]] | ||
|
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.
Why is this cache separate from the Parser
's? Should it be a single cache at the Interpreter
level?
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.
Changed, there's now a single cache at the interpreter level.
sjsonnet/src/sjsonnet/Val.scala
Outdated
@@ -297,15 +298,45 @@ object Val{ | |||
} | |||
} | |||
|
|||
def staticObject(pos: Position, fields: Array[Expr.Member.Field]): Obj = { | |||
final case class StaticObjectFieldSet(keys: Array[String]) { |
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.
This shouldn't be a case class. Array
has no useful equality or toString
and you're overriding equals
and hashCode
.
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.
Fixed
Sorry I missed your most important question. The performance impact here was undetectable in the benchmark. I think the fact that the object interning is thread-local and unsynchronized makes it pretty fast. |
@@ -3,6 +3,8 @@ package sjsonnet | |||
import java.io.StringWriter | |||
import java.util.concurrent.TimeUnit | |||
|
|||
import scala.collection.mutable.HashMap |
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.
I'd prefer to keep mutable types qualified (i.e. only import scala.collection.mutable
) everywhere for consistency.
Before: 855MB for the parsed file
After: 425MB