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

Add AffinityPool which pins actors to threads #23104

Merged
merged 33 commits into from Jul 7, 2017

Conversation

Projects
None yet
7 participants
@zaharidichev
Contributor

zaharidichev commented Jun 5, 2017

As already discussed in #12035 , this commit adds a new kind of thread pool called AffinityPool. Its a very early proof of concept fixed size thread pool which has two levels of pinning. Firstly, it executes an actor's Mailbox on the same thread it used to run it. Secondly, it is configurable to use any of the following strategies to allow for thread - CPU pinning:

  • any
  • same-core
  • same-socket
  • different-core
  • different-socket

As an early prototype, this implementation suffers from quite a few race conditions around shutting down, restarting workers but all of that shall be handled if the decision is taken to proceed with this work.

Under the hood it uses Open HFT affinity Library. At the moment, I am running Mac OS, so I cannot pin threads to cores, etc as Mac Os does not expose taskset. However even with actor to thread pinning, we get some pretty interesting results and particularly quite the boost in throughput by keeping the fairness of the dispatcher high:

affinity-boost

Currently there are two benchmarks that can be run:

  • AffinityPoolComparativeBenchmark - runs an actor benchmark and compares the throughput of the affinity, FJ and a fixed thread size pool by varying their throughput param.
  • CPUAffinityStrategiesBenchmark - runs an actor benchmark and compares how the different Affinity strategies reflect onto the throughput of the dispatcher.

Run with:

jmh:run -i 15 -wi 7 -f1 -t1 akka.actor.AffinityPoolComparativeBenchmark

and

jmh:run -i 15 -wi 7 -f1 -t1 akka.actor.CPUAffinityStrategiesBenchmark

Add AffinityPool which pins actors to threads
Add benchmakarks that compares the AffinityPool against FJ and Fixed Size pools
Refactor ForkJoinActorBenchmark, pulled actors and helper methods in separate file so they can be easily reusable in similar benchmarks
Add benchmark to measure the effects of different affinity CPU strategies
@akka-ci

This comment has been minimized.

Show comment
Hide comment
@akka-ci

akka-ci Jun 7, 2017

Collaborator

Can one of the repo owners verify this patch?

Collaborator

akka-ci commented Jun 7, 2017

Can one of the repo owners verify this patch?

@jrudolph

This comment has been minimized.

Show comment
Hide comment
@jrudolph

jrudolph Jun 7, 2017

Member

OK TO TEST

Member

jrudolph commented Jun 7, 2017

OK TO TEST

@akka-ci

This comment has been minimized.

Show comment
Hide comment
@akka-ci

akka-ci Jun 7, 2017

Collaborator

Test FAILed.

Collaborator

akka-ci commented Jun 7, 2017

Test FAILed.

@zaharidichev

This comment has been minimized.

Show comment
Hide comment
@zaharidichev

zaharidichev Jun 7, 2017

Contributor

Is there any particular reason dependency net.openhft affinity 3.0.6 cannot be resolved on the CI server ?

Its an Apache license so should be good

Contributor

zaharidichev commented Jun 7, 2017

Is there any particular reason dependency net.openhft affinity 3.0.6 cannot be resolved on the CI server ?

Its an Apache license so should be good

@patriknw

This comment has been minimized.

Show comment
Hide comment
@patriknw

patriknw Jun 7, 2017

Member

We have problems with the artifactory proxy, for some reason it fails the first attempt to download new dependencies. It will probably work next time. I'll start a rebuild.

Member

patriknw commented Jun 7, 2017

We have problems with the artifactory proxy, for some reason it fails the first attempt to download new dependencies. It will probably work next time. I'll start a rebuild.

@patriknw

This comment has been minimized.

Show comment
Hide comment
@patriknw

patriknw Jun 7, 2017

Member

PLS BUILD

Member

patriknw commented Jun 7, 2017

PLS BUILD

@akka-ci

This comment has been minimized.

Show comment
Hide comment
@akka-ci

akka-ci Jun 7, 2017

Collaborator

Test FAILed.

Collaborator

akka-ci commented Jun 7, 2017

Test FAILed.

@akka-ci

This comment has been minimized.

Show comment
Hide comment
@akka-ci

akka-ci Jul 4, 2017

Collaborator

Test FAILed.

Collaborator

akka-ci commented Jul 4, 2017

Test FAILed.

@akka-ci

This comment has been minimized.

Show comment
Hide comment
@akka-ci

akka-ci Jul 4, 2017

Collaborator

Test FAILed.

Collaborator

akka-ci commented Jul 4, 2017

Test FAILed.

@akka-ci akka-ci added validating and removed needs-attention labels Jul 4, 2017

@akka-ci akka-ci added tested and removed validating labels Jul 4, 2017

@akka-ci

This comment has been minimized.

Show comment
Hide comment
@akka-ci

akka-ci Jul 4, 2017

Collaborator

Test PASSed.

Collaborator

akka-ci commented Jul 4, 2017

Test PASSed.

Show outdated Hide outdated akka-actor/src/main/resources/reference.conf
Show outdated Hide outdated akka-actor/src/main/scala/akka/util/ImmutableIntMap.scala
private var left = numQueries
private val receivedUsers: mutable.Map[Int, User] = mutable.Map()
private val randGenerator = new Random()

This comment has been minimized.

@patriknw

patriknw Jul 5, 2017

Member

well, I don't expect that we see difference in benchmarks. Random is known to not be very fast (it's synchronized and such), and I think it has a bigger problem of blocking when OS is running out of entropy.

@patriknw

patriknw Jul 5, 2017

Member

well, I don't expect that we see difference in benchmarks. Random is known to not be very fast (it's synchronized and such), and I think it has a bigger problem of blocking when OS is running out of entropy.

@patriknw

LGTM, great work!

@akka-ci akka-ci added validating and removed tested labels Jul 5, 2017

@zaharidichev

This comment has been minimized.

Show comment
Hide comment
@zaharidichev

zaharidichev Jul 5, 2017

Contributor

@patriknw @viktorklang

Thanks a lot for the support and time. If you need anything else before giving another LGTM. just ping me here :) I think most of the remarks have been addressed

Contributor

zaharidichev commented Jul 5, 2017

@patriknw @viktorklang

Thanks a lot for the support and time. If you need anything else before giving another LGTM. just ping me here :) I think most of the remarks have been addressed

@akka-ci akka-ci added tested and removed validating labels Jul 5, 2017

@akka-ci

This comment has been minimized.

Show comment
Hide comment
@akka-ci

akka-ci Jul 5, 2017

Collaborator

Test PASSed.

Collaborator

akka-ci commented Jul 5, 2017

Test PASSed.

@patriknw

This comment has been minimized.

Show comment
Hide comment
@patriknw

patriknw Jul 7, 2017

Member

Reviewed by Viktor and me, so merging.

Thanks again @zaharidichev
If you would like to pick up something more the PinnedDispatcher or ManyToOneArrayMailbox I mentioned earlier might be good, or ping us if you have any other ideas.

Member

patriknw commented Jul 7, 2017

Reviewed by Viktor and me, so merging.

Thanks again @zaharidichev
If you would like to pick up something more the PinnedDispatcher or ManyToOneArrayMailbox I mentioned earlier might be good, or ping us if you have any other ideas.

@patriknw patriknw merged commit 4d45064 into akka:master Jul 7, 2017

2 checks passed

Jenkins PR Validation Test PASSed. 13832 tests run, 501 skipped, 0 failed.
Details
typesafe-cla-validator All users have signed the CLA
Details
@viktorklang

This comment has been minimized.

Show comment
Hide comment
@viktorklang

viktorklang Jul 7, 2017

Member

Excellent work, @zaharidichev. Thanks for putting up with all of our review comments!

Member

viktorklang commented Jul 7, 2017

Excellent work, @zaharidichev. Thanks for putting up with all of our review comments!

@zaharidichev

This comment has been minimized.

Show comment
Hide comment
@zaharidichev

zaharidichev Jul 7, 2017

Contributor

@patriknw @viktorklang Thanks a lot for the support. Your review comments were very usful and I ma quite happy you like the work I did. I will take a look at #23104 (comment) shortly and let you know how it goes.

Contributor

zaharidichev commented Jul 7, 2017

@patriknw @viktorklang Thanks a lot for the support. Your review comments were very usful and I ma quite happy you like the work I did. I will take a look at #23104 (comment) shortly and let you know how it goes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment