Skip to content

Reduce performance overhead of map lookups in Invoker.invoked() #202

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.

Sign up for GitHub

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

Merged
merged 2 commits into from
Jul 5, 2018
Merged

Reduce performance overhead of map lookups in Invoker.invoked() #202

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
78d0b13
Avoid unnecessary StringBuilder construction and int boxing.
JoshRosen Jun 9, 2017
4626fd8
Use two levels of maps to avoid hashing tuples on every lookup.
JoshRosen Jun 12, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 19 additions & 6 deletions scalac-scoverage-runtime/shared/src/main/scala/scoverage/Invoker.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,12 @@ import scoverage.Platform._
object Invoker {

private val MeasurementsPrefix = "scoverage.measurements."
private val threadFiles = new ThreadLocal[ThreadSafeMap[String, FileWriter]]
private val ids = ThreadSafeMap.empty[(String, Int), Any]
private val threadFiles = new ThreadLocal[mutable.HashMap[String, FileWriter]]

// For each data directory we maintain a thread-safe set tracking the ids that we've already
// seen and recorded. We're using a map as a set, so we only care about its keys and can ignore
// its values.
private val dataDirToIds = ThreadSafeMap.empty[String, ThreadSafeMap[Int, Any]]

/**
* We record that the given id has been invoked by appending its id to the coverage
Expand All @@ -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".
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

dataDirToIds.synchronized {
if (!dataDirToIds.contains(dataDir)) {
dataDirToIds(dataDir) = ThreadSafeMap.empty[Int, Any]
}
}
}
val ids = dataDirToIds(dataDir)
if (!ids.contains(id)) {
// Each thread writes to a separate measurement file, to reduce contention
// and because file appends via FileWriter are not atomic on Windows.
var files = threadFiles.get()
if (files == null) {
files = ThreadSafeMap.empty[String, FileWriter]
files = mutable.HashMap.empty[String, FileWriter]
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()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.


ids.put((dataDir, id), ())
ids.put(id, ())
}
}

Expand Down