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

Performance regression for events without tags #586

Merged
merged 8 commits into from
Aug 30, 2021

Conversation

johanandren
Copy link
Member

@johanandren johanandren commented Aug 27, 2021

References #585

akka.persistence.jdbc.integration.PostgresJournalPerfSpec on master against local docker postgres:

[info] A PersistentActor's performance
[info] - must measure: persist()-ing 100 events for 100 actors (10 seconds, 218 milliseconds)
[info]   + Persist()-ing 100 * 100 took 10184 ms
[info]   + Average time: 10184 ms
[info] A PersistentActor's performance
[info] - must measure: persistAsync()-ing 100 events for 100 actors (3 seconds, 984 milliseconds)
[info]   + persistAsync()-ing 100 * 100 took 3955 ms
[info]   + Average time: 3955 ms

With this fix, back to pre-5.0.0 levels:

[info] A PersistentActor's performance
[info] - must measure: persist()-ing 100 events for 100 actors (2 seconds, 244 milliseconds)
[info]   + Persist()-ing 100 * 100 took 2213 ms
[info]   + Average time: 2213 ms
[info] A PersistentActor's performance
[info] - must measure: persistAsync()-ing 100 events for 100 actors (441 milliseconds)
[info]   + persistAsync()-ing 100 * 100 took 408 ms
[info]   + Average time: 408 ms

Note that tagged events (and batches with tagged events in them) will still be in the slow branch of writing.

} else {
// optimization avoid some work when not using tags
val events = sorted.map(_._1)
JournalTableC ++= events
Copy link
Member Author

Choose a reason for hiding this comment

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

This fastpath branch when there are no tags and being able to use the Compiled(table) the actual fix, the other stuff is just some small optimization housekeeping that doesn't hurt but also doesn't speed things up significantly.

Copy link
Member

Choose a reason for hiding this comment

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

It's a pity that we still have the penalty when using tags. I'm wondering if it's worth to try to improve it further.

We have being discussing the idea of adding first-class support for journal slicing. If we go ahead with that idea, tags will become a less popular feature. And journal slicing won't need to write to another table.

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, I added a note about that in the issue, perhaps that can be worked on separately.

implicit val mat: Materializer
implicit val ec: ExecutionContext

def baseDaoConfig: BaseDaoConfig

lazy val writeQueue: SourceQueueWithComplete[(Promise[Unit], Seq[T])] = Source
val writeQueue: SourceQueueWithComplete[(Promise[Unit], Seq[T])] = Source
Copy link
Member Author

Choose a reason for hiding this comment

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

Avoiding a lock

@@ -82,11 +82,9 @@ class JdbcAsyncWriteJournal(config: Config) extends AsyncWriteJournal {

override def asyncWriteMessages(messages: Seq[AtomicWrite]): Future[Seq[Try[Unit]]] = {
// add timestamp to all payloads in all AtomicWrite messages
val now = System.currentTimeMillis()
Copy link
Member Author

Choose a reason for hiding this comment

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

Avoiding multiple syscalls that likely would give the same ms back anyway

@@ -41,7 +42,7 @@ class DefaultJournalDao(
override def baseDaoConfig: BaseDaoConfig = journalConfig.daoConfig

override def writeJournalRows(xs: immutable.Seq[(JournalAkkaSerializationRow, Set[String])]): Future[Unit] = {
db.run(queries.writeJournalRows(xs).transactionally).map(_ => ())
db.run(queries.writeJournalRows(xs).transactionally).map(_ => ())(ExecutionContexts.parasitic)
Copy link
Member Author

Choose a reason for hiding this comment

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

Avoid execution context jump just to "silence" future return value

Copy link
Member

@octonato octonato left a comment

Choose a reason for hiding this comment

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

LGTM

@octonato octonato merged commit 6f613b3 into master Aug 30, 2021
@octonato octonato deleted the wip-585-perf-regression branch August 30, 2021 13:34
@octonato octonato modified the milestones: 5.1.0, 5.0.3 Aug 30, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants