Reduce performance overhead of map lookups in Invoker.invoked() #202
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
JoshRosen
commented
Jun 12, 2017
| threadFiles.set(files) | ||
| } | ||
| val writer = files.getOrElseUpdate(dataDir, new FileWriter(measurementFile(dataDir), true)) | ||
| writer.append(id.toString + '\n').flush() | ||
| writer.append(Integer.toString(id)).append("\n").flush() |
There was a problem hiding this comment.
I'm pretty sure that id.toString boxes here, so Integer.toString(...) saves that overhead.
The key change on this line is to replace the string concatenation with two separate .append() calls. The concatenation ends up implicitly constructing a StringBuilder.
JoshRosen
commented
Jun 12, 2017
| @@ -31,18 +35,27 @@ object Invoker { | |||
| // times since for coverage we only care about 1 or more, (it just slows things down to | |||
| // do it more than once), anything we can do to help is good. This helps especially with code | |||
| // that is executed many times quickly, eg tight loops. | |||
| if (!ids.contains(dataDir, id)) { | |||
| if (!dataDirToIds.contains(dataDir)) { | |||
| // Guard against SI-7943: "TrieMap method getOrElseUpdate is not thread-safe". | |||
There was a problem hiding this comment.
I noticed that this was also calling getOrElseUpdate on a TrieMap down in files.getOrElseUpdate, but that case wasn't an issue because that map is used only in a single thread (since it comes from a non-inheritable ThreadLocal).
To help make this clearer in the code, I went ahead and used a plain mutable.HashMap at that other site.
|
👍 |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Invoker.invoked()is showing up as a performance hotspot in my tests. For full details of my investigation, see #201.In my profiling, half of the time in
Invoker.invoked()was spent computing hashCodes when indexing into theidsmap, which is aThreadSafeMap[(String, Int), Any]. This PR replaces the single-level map by a two-levelThreadSafeMap[String, ThreadSafeMap[Int, Any]]map. This isn't much more space because theStringpart of the key is usually constant and it's not a whole lot of extra lookup time either because the outer map typically has at most one element.After making this change I no longer saw map lookups as a bottleneck; instead, all of the time shifted to
flush()calls (which I plan to address in a separate PR; see discussion at #201).I also made a small micro-optimization to avoid unnecessary int boxing and implicit
StringBuilderconstruction.