Skip to content

Fix asymptotic performance issues in live variables analysis. #82

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 1 commit into from
Jul 18, 2014
Merged

Fix asymptotic performance issues in live variables analysis. #82

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
71 changes: 43 additions & 28 deletions src/main/scala/scala/async/internal/LiveVariables.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -126,14 +126,22 @@ trait LiveVariables {

/** Tests if `state1` is a predecessor of `state2`.
*/
def isPred(state1: Int, state2: Int, seen: Set[Int] = Set()): Boolean =
if (seen(state1)) false // breaks cycles in the CFG
else cfg get state1 match {
case Some(nextStates) =>
nextStates.contains(state2) || nextStates.exists(isPred(_, state2, seen + state1))
case None =>
false
}
def isPred(state1: Int, state2: Int): Boolean = {
val seen = scala.collection.mutable.HashSet[Int]()

def isPred0(state1: Int, state2: Int): Boolean =
if(state1 == state2) false
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems to be the fix for the root problem. Correct?

Copy link
Contributor

Choose a reason for hiding this comment

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

From my testing, the three changes should all independently have nontrivial, measurable performance improvement (possibly with increasing the nesting depth of the example).

Copy link
Contributor

Choose a reason for hiding this comment

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

I agree with your assessment now.

Copy link
Member Author

Choose a reason for hiding this comment

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

Me too, I applied the changes one at a time and saw speedups for each of them.

LGTM

else if (seen(state1)) false // breaks cycles in the CFG
else cfg get state1 match {
case Some(nextStates) =>
seen += state1
nextStates.contains(state2) || nextStates.exists(isPred0(_, state2))
case None =>
false
}

isPred0(state1, state2)
}

val finalState = asyncStates.find(as => !asyncStates.exists(other => isPred(as.state, other.state))).get

Expand Down Expand Up @@ -162,12 +170,10 @@ trait LiveVariables {
LVexit = LVexit + (finalState.state -> noNull)

var currStates = List(finalState) // start at final state
var pred = List[AsyncState]() // current predecessor states
var hasChanged = true // if something has changed we need to continue iterating
var captured: Set[Symbol] = Set()

while (hasChanged) {
hasChanged = false
while (!currStates.isEmpty) {
var entryChanged: List[AsyncState] = Nil

for (cs <- currStates) {
val LVentryOld = LVentry(cs.state)
Expand All @@ -176,44 +182,53 @@ trait LiveVariables {
val LVentryNew = LVexit(cs.state) ++ referenced.used
if (!LVentryNew.sameElements(LVentryOld)) {
LVentry = LVentry + (cs.state -> LVentryNew)
hasChanged = true
entryChanged ::= cs
}
}

pred = currStates.flatMap(cs => asyncStates.filter(_.nextStates.contains(cs.state)))
val pred = entryChanged.flatMap(cs => asyncStates.filter(_.nextStates.contains(cs.state)))
var exitChanged: List[AsyncState] = Nil

for (p <- pred) {
val LVexitOld = LVexit(p.state)
val LVexitNew = p.nextStates.flatMap(succ => LVentry(succ)).toSet
if (!LVexitNew.sameElements(LVexitOld)) {
LVexit = LVexit + (p.state -> LVexitNew)
hasChanged = true
exitChanged ::= p
}
}

currStates = pred
currStates = exitChanged
}

for (as <- asyncStates) {
AsyncUtils.vprintln(s"LVentry at state #${as.state}: ${LVentry(as.state).mkString(", ")}")
AsyncUtils.vprintln(s"LVexit at state #${as.state}: ${LVexit(as.state).mkString(", ")}")
}

def lastUsagesOf(field: Tree, at: AsyncState, avoid: Set[AsyncState]): Set[Int] =
if (avoid(at)) Set()
else if (captured(field.symbol)) {
Set()
}
else LVentry get at.state match {
case Some(fields) if fields.exists(_ == field.symbol) =>
Set(at.state)
case _ =>
val preds = asyncStates.filter(_.nextStates.contains(at.state)).toSet
preds.flatMap(p => lastUsagesOf(field, p, avoid + at))
def lastUsagesOf(field: Tree, at: AsyncState): Set[Int] = {
val avoid = scala.collection.mutable.HashSet[AsyncState]()
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems this optimization (turning the avoid parameter into a local variable, which is a mutable hash set) is independent of the rest of this change set. If so, what's the performance improvement? The code is slightly longer and more imperative, so we might want to have a good reason to do it. What do people think?

Copy link
Contributor

Choose a reason for hiding this comment

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

My interpretation of this code is that it's a standard depth-first search through the state graph, just like isPred. If I recall correctly, without this change there is measurably exponential runtime growth when adding an extra level of nesting in the example, which would occur if the state graph has repeatedly-chained 'diamonds', e.g.

1->2
1->3
2->4
3->4

and so forth.

The immutable version doesn't remember that it's traversed the entire subgraph starting from 4, so ends up exploring the exponential number of paths from 1 to the final state.

Copy link
Contributor

Choose a reason for hiding this comment

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

That clarifies it- thanks! So, yes, this optimization is definitely essential.


def lastUsagesOf0(field: Tree, at: AsyncState): Set[Int] = {
if (avoid(at)) Set()
else if (captured(field.symbol)) {
Set()
}
else LVentry get at.state match {
case Some(fields) if fields.exists(_ == field.symbol) =>
Set(at.state)
case _ =>
avoid += at
val preds = asyncStates.filter(_.nextStates.contains(at.state)).toSet
preds.flatMap(p => lastUsagesOf0(field, p))
}
}

lastUsagesOf0(field, at)
}

val lastUsages: Map[Tree, Set[Int]] =
liftables.map(fld => (fld -> lastUsagesOf(fld, finalState, Set()))).toMap
liftables.map(fld => (fld -> lastUsagesOf(fld, finalState))).toMap

for ((fld, lastStates) <- lastUsages)
AsyncUtils.vprintln(s"field ${fld.symbol.name} is last used in states ${lastStates.mkString(", ")}")
Expand Down
77 changes: 77 additions & 0 deletions src/test/scala/scala/async/run/ifelse1/IfElse1.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,75 @@ class TestIfElse1Class {
}
z
}

def pred: Future[Boolean] = async(true)

def m5: Future[Boolean] = async {
if(if(if(if(if(if(if(if(if(if(if(if(if(if(if(if(if(if(if(if(if(await(pred))
await(pred)
else
false)
await(pred)
else
false)
await(pred)
else
false)
await(pred)
else
false)
await(pred)
else
false)
await(pred)
else
false)
await(pred)
else
false)
await(pred)
else
false)
await(pred)
else
false)
await(pred)
else
false)
await(pred)
else
false)
await(pred)
else
false)
await(pred)
else
false)
await(pred)
else
false)
await(pred)
else
false)
await(pred)
else
false)
await(pred)
else
false)
await(pred)
else
false)
await(pred)
else
false)
await(pred)
else
false)
await(pred)
else
false
}
}

class IfElse1Spec {
Expand Down Expand Up @@ -124,4 +193,12 @@ class IfElse1Spec {
val res = Await.result(fut, 2 seconds)
res mustBe (14)
}

@Test
def `await in deeply-nested if-else conditions`() {
val o = new TestIfElse1Class
val fut = o.m5
val res = Await.result(fut, 2 seconds)
res mustBe true
}
}