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

optimize scope delegation #4003

Merged
merged 3 commits into from Jun 27, 2018

Conversation

Projects
None yet
4 participants
@eed3si9n
Copy link
Member

eed3si9n commented Mar 11, 2018

This is a refactoring of @jrudolph's #3979 on top of test PR #4002

For every setting, we need to calculate the delegates, so indexedDelegates is one of hot methods.

build sbt 1.1.1 1.1.2 this patch
Cinnamon (125070 settings) 35.539 ± 0.474s 35.469 ± 0.318s 32.242 ± 0.990s
Akka (30398 settings) 17.908 ± 0.101s 17.780 ± 1.494s 17.166 ± 0.726s

Given that 1.1.x branch doesn't have optimization changes merged in we can ignore the difference between sbt 1.1.1 and 1.1.x. Basically it's around 35.5s within a second of waffling.
With this patch, I am getting around 32.2, which is 3s speedup.

As you can see, with Akka's build with much fewer settings there's roughly 0.6s speedup, so this patch likely won't help you much until you have a lot of subprojects.

In any case, 7s would be the lion's share of Johannes's ~10s speedup. (In earlier version, I had user CPU time confused with the total) I focused on just the delegation expansion, so this change is a lot more contained.

eed3si9n added some commits Mar 10, 2018

handroll for-expression
Ref #3979

Based on #3979 this handrolls the for-express using while.
don't use toStream
Ref #3979
toStream doesn't help performance.

@eed3si9n eed3si9n requested a review from dwijnand Mar 11, 2018

case Select(conf) => index.config(configProj, conf); case _ => withZeroAxis(scope.config)
}
// This is a hot method that gets called many times
def exandDelegateScopes(resolvedProj: ResolvedReference)(

This comment has been minimized.

@dwijnand

dwijnand Mar 12, 2018

Member

expand


def definingScope(scope: Scope, key: AttributeKey[_]): Option[Scope] =
delegates(scope).toStream.find(sc => getDirect(sc, key).isDefined)
delegates(scope).find(sc => getDirect(sc, key).isDefined)

This comment has been minimized.

@dwijnand

dwijnand Mar 12, 2018

Member

do they help memory consumption though, by being lazy?

This comment has been minimized.

@eed3si9n

eed3si9n Mar 13, 2018

Author Member

Assuming find returns at first hit, I am not sure if this saves either time or space.

This comment has been minimized.

@eed3si9n

eed3si9n Mar 13, 2018

Author Member

If we had an iterator version, as suggested by Jason below, it would be a different story. But I'd say that would be another PR.

This comment has been minimized.

This comment has been minimized.

@jrudolph

jrudolph Mar 13, 2018

Member

Delegation chain shouldn't be too long (< 100 elements usually) and the objects are short-lived, so without a bteter idea like the iterator, I'd also say, the extra memory usage doesn't matter here.

For find the big problem was the aggregation caching because in most cases aggregate in X is not defined but on the end of the delegation chain in which case using Stream was super expensive.

// projAxes flatMap nonProjectScopes(resolvedProj)
// ...
// for (c <- cLin; t <- tLin; e <- eLin) yield Scope(px, c, t, e)
val res = Vector.newBuilder[Scope]

This comment has been minimized.

@retronym

retronym Mar 13, 2018

Member

I'm not sure Vector is the best choice here. ListBuffer would be the same as used before, it might make sense to use that first, and the separately try a different SeqBuilder. When I was looking at speeding up this code, I tried a pre-sized ArrayBuffer, I think.

This comment has been minimized.

@retronym

retronym Mar 13, 2018

Member

But a big 👍 for getting rid of the nested flatMap-s, every level of nesting doubles the amount of garbage created.

This comment has been minimized.

@retronym

retronym Mar 13, 2018

Member

The ideal structure would actually be a hand-rolled Iterator[Scope], but is a huge pain to refactor into that form.

This comment has been minimized.

@eed3si9n

eed3si9n Mar 13, 2018

Author Member

I went for Vector.newBuilder based on https://stackoverflow.com/a/14059155/3827. @jrudolph went for the presized val res = new ArrayBuffer[Scope](pLin.size * 2 * tLin.size * eLin.size), but I didn't see observable difference, so I figured that no longer became the bottleneck.

This comment has been minimized.

@eed3si9n

eed3si9n Mar 13, 2018

Author Member

The ideal structure would actually be a hand-rolled Iterator[Scope], but is a huge pain to refactor into that form.

That might be a good idea for hackathon to add an iterator version.

This comment has been minimized.

@jrudolph

jrudolph Mar 13, 2018

Member

The ideal structure would actually be a hand-rolled Iterator[Scope], but is a huge pain to refactor into that form.

👍

@jrudolph

This comment has been minimized.

Copy link
Member

jrudolph commented Mar 13, 2018

Thanks for taking that one for a spin and adding more tests, @eed3si9n!

@jrudolph

This comment has been minimized.

Copy link
Member

jrudolph commented Mar 13, 2018

Btw. on my 5 year old computer, Akka loads in 20 seconds, so there might be something else going on on your machine with the Akka build.

@eed3si9n

This comment has been minimized.

Copy link
Member Author

eed3si9n commented Mar 13, 2018

Btw. on my 5 year old computer, Akka loads in 20 seconds, so there might be something else going on on your machine with the Akka build.

Interesting. Maybe global settings causing this or global plugins (I only have addSbtPlugin("com.jsuereth" % "sbt-pgp" % "1.1.0") at the moment), but that's a big difference.

@eed3si9n

This comment has been minimized.

Copy link
Member Author

eed3si9n commented Mar 19, 2018

@dwijnand Can we put this into 1.1.2, or is there a blocker?

@dwijnand

This comment has been minimized.

Copy link
Member

dwijnand commented Mar 20, 2018

I too would love to have this in 1.1.2, but given the importance of scope delegation the fact that it isn't properly tested makes me wary of merging these kinds of refactorings without even a RC.

so imo our choices are either:

  • complete the retrofitting of test coverage for the scope delegation (aka #4002), or
  • cross our fingers and merge this into 1.x, and then make sure we really test 1.2.0-RC1 to make sure we haven't broken anything

I don't, at this time, have a strong preference for or against delaying sbt 1.1.2 for this change.

def withZeroAxis[T](base: ScopeAxis[T]): Seq[ScopeAxis[T]] =
if (base.isSelect) base :: Zero :: Nil
else Zero :: Nil
if (base.isSelect) List(base, Zero)

This comment has been minimized.

@dwijnand

dwijnand Mar 21, 2018

Member

btw, this is slower than the previous implementation because this one requires an Array allocation of List.apply's varargs..

This comment has been minimized.

@retronym

retronym Mar 21, 2018

Member

Even better, the Zero :: Nil (also GlobalScope :: Nil) part can be shared, as I did in retronym@45b170e

@retronym

This comment has been minimized.

Copy link
Member

retronym commented Mar 21, 2018

so imo our choices are either:

One approach that is useful when test coverage isn't great is to include a commit in the PR that includes the old and new implementations, and asserts the results are the same. You can run that commit through some real-world builds to check the tripwire never goes off.

@dwijnand

This comment has been minimized.

Copy link
Member

dwijnand commented Mar 21, 2018

@retronym interesting idea. I'll try that out.

@eed3si9n

This comment has been minimized.

Copy link
Member Author

eed3si9n commented Mar 28, 2018

Btw. on my 5 year old computer, Akka loads in 20 seconds, so there might be something else going on on your machine with the Akka build.

Finally figured this out. noob mistake on my part. Given sbt exit 43.68s user 4.22s system 247% cpu 19.323 total I was reading the user CPU time.

@eed3si9n eed3si9n changed the base branch from 1.1.x to 1.x Jun 27, 2018

@eed3si9n

This comment has been minimized.

Copy link
Member Author

eed3si9n commented Jun 27, 2018

Merging this into 1.x so we can test it out during 1.2 RC cycle.

@eed3si9n eed3si9n merged commit eb942f8 into sbt:1.x Jun 27, 2018

2 of 3 checks passed

Codacy/PR Quality Review Not so good... This pull request quality could be better.
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@eed3si9n eed3si9n deleted the eed3si9n:wip/opt-delegation3 branch Jun 27, 2018

@eed3si9n eed3si9n removed the in progress label Jun 27, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.