Skip to content
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

[compiler] Minor Requiredness Performance Enchancements #13991

Merged
merged 7 commits into from
Nov 16, 2023
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
35 changes: 32 additions & 3 deletions hail/src/main/scala/is/hail/expr/ir/Requiredness.scala
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,7 @@ class Requiredness(val usesAndDefs: UsesAndDefs, ctx: ExecuteContext) {
type State = Memo[BaseTypeWithRequiredness]
private val cache = Memo.empty[BaseTypeWithRequiredness]
private val dependents = Memo.empty[mutable.Set[RefEquality[BaseIR]]]
private val q = mutable.Set[RefEquality[BaseIR]]()
private[this] val q = new Queue()

private val defs = Memo.empty[IndexedSeq[BaseTypeWithRequiredness]]
private val states = Memo.empty[IndexedSeq[TypeWithRequiredness]]
Expand Down Expand Up @@ -90,8 +90,7 @@ class Requiredness(val usesAndDefs: UsesAndDefs, ctx: ExecuteContext) {

def run(): Unit = {
while (q.nonEmpty) {
val node = q.head
q -= node
val node = q.pop()
if (analyze(node.t) && dependents.contains(node)) {
q ++= dependents.lookup(node)
}
Expand Down Expand Up @@ -828,4 +827,34 @@ class Requiredness(val usesAndDefs: UsesAndDefs, ctx: ExecuteContext) {

requiredness.probeChangedAndReset()
}

// "Oh god, why? Why not just use a HashSet like a normal person?" I hear you ask.
// Well, it turns out that half of the time spent in `Requiredness` for large IRs
// would be spent removing items from a HashSet.
// Go on, profile it.
// Be as astonished as I was.
final class Queue {
private[this] val q =
mutable.Queue[RefEquality[BaseIR]]()
private[this] val seen =
mutable.AnyRefMap[RefEquality[BaseIR], Int]()
Copy link
Collaborator

Choose a reason for hiding this comment

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

Probably no performance difference, but it would be clearer for this to map to Boolean instead of Int. And maybe rename this to inQueue? seen sounds like it includes things that are no longer in the queue.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, good suggestion.


def nonEmpty: Boolean =
q.nonEmpty

def pop(): RefEquality[BaseIR] = {
val n = q.dequeue()
seen.update(n, 0)
n
}

def +=(re: RefEquality[BaseIR]): Unit =
if (0 == seen.getOrElse(re, 0)) {
seen.update(re, 1)
q += re
}

def ++=(res: Iterable[RefEquality[BaseIR]]): Unit =
res.foreach(this += _)
}
}
7 changes: 5 additions & 2 deletions hail/src/main/scala/is/hail/types/TypeWithRequiredness.scala
Original file line number Diff line number Diff line change
Expand Up @@ -98,8 +98,11 @@ sealed abstract class BaseTypeWithRequiredness {
throw new AssertionError(
s"children lengths differed ${children.length} ${newChildren.length}. ${children} ${newChildren} ${this}")
}
(children, newChildren).zipped.foreach { (r1, r2) =>
r1.unionFrom(r2)

// foreach on zipped seqs is very slow as the implementation
// doesn't know that the seqs are the same length.
Comment on lines +102 to +103
Copy link
Collaborator

Choose a reason for hiding this comment

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

I feel like this comment is out of place here. We use both of these patterns all over the codebase, and we shouldn't comment on their relative performance every time. Perhaps we should start a doc of these kinds of scala performance gotchas that we can refer to, and reevaluate with future scala version changes.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for your feedback.

A zip and foreach is more like the code I want to write and indeed what I will write when not in a hotspot. A comment will prevent my future self getting upset at whomever wrote this low level ugly crap and changing it back.

I don't think a doc would be that practical. Knowing me, I'd likely forget about it rather than consult it whenever I need to write simple code like a for-loop.

for (i <- children.indices) {
children(i).unionFrom(newChildren(i))
}
}

Expand Down