Skip to content

Let a waiting committer lead the group commit - #72

Open
venkat1701 wants to merge 3 commits into
mainfrom
perf/leader-commit
Open

venkat1701 wants to merge 3 commits into
mainfrom
perf/leader-commit

Conversation

@venkat1701

Copy link
Copy Markdown
Collaborator

Part of #46

What was slow: most of a small commit's time wasn't work, it was waiting. Every commit handed its batch to the dedicated hstore-group-commit thread and parked until that thread woke it again. That's two thread wake-ups per commit, even under async durability where nothing is synced. JFR on 8,100 single-update commits shows the committing thread parked about once per commit, for 1.1 ms on average, which was roughly 80% of the commit time on this loaded machine.

What changed (leader commits, as agreed):

  • There is no group-commit thread any more.
  • The first committer waiting on an unpublished commit becomes the leader. It takes everything queued, makes it durable once, and publishes it in generation order. Then it wakes the others.
  • A lone committer finishes on its own thread with no hand-off. Commits that arrive while a leader is syncing are published together by the next leader, still sharing one fsync.
  • Failures behave as before: stored, logged, and rethrown to the leader and to every waiting and later committer. Simulated crashes from the fault tests reach the committer the same way.
  • drain() and close() lead too, so nothing stays queued.

One trade-off: the fsync now runs on the committing thread, which in the server is usually a virtual thread. The JDK copes with a virtual thread blocked in file I/O by adding a carrier temporarily. That's in the docs.

Results, HStore only, scale 1, async, 3 alternating runs each against main (2094e92), all checksums identical:

before after
single-update commit p50 276 µs 213 µs (-23%)
single-update commit p99 1,067 µs 601 µs (-44%)
ingest.nodes 21.7k/s 25.0k/s (+15%)
write.update 20.2k/s 22.3k/s (+10%)
reads unchanged

Caveat about mixed.write p99: in 2 of 3 runs after the change it went from about 240 ms to 3.7 to 4.4 s. I traced it with JFR, and it isn't this change. The writer waited 4.4 s for the commit lock, which background compaction held during TransactionManager.rewrite (#50). Every run, before and after, had exactly one background compaction pass. In the slow runs it fell inside the 10 s mixed window, which is why their mixed window wrote 117 MiB instead of 88. When it fell elsewhere, the new code did better than the old (p99 95 and 146 ms against 233 to 240 ms). Faster commits shift when the bytes-written trigger fires, so it lands in the window more often. The fix is #50, which I'm doing next.

Tested:

  • full ./mvnw install passes (95 tests); each commit compiles on its own
  • CrashRecoveryTest (16 crash points × WAL modes) passed 5 runs in a row
  • new GroupCommitterTest:
    • a lone commit is published on its own thread (this one fails on the old committer)
    • 1,600 commits from 8 threads come out in exact submission order, sharing barriers
    • a failed barrier fails that commit and every later one
  • native Docker image with the default SYNC durability:
    • loads clinical-claims.hql through sync commits
    • healthy, restarts healthy, three clean shutdowns
    • hstore check passes; all 24 patients present

Also fixes the group-commit section of transactions.md, which still said the feed is appended before the generation becomes current; #49 reversed that.

Every commit used to hand its batch to a dedicated group-commit thread
and wait to be woken again, even under async durability where nothing
is synced. That was two thread wake-ups per commit. Now the first
committer waiting on an unpublished commit leads: it syncs once for
everything queued and publishes it in order, while the others wait. A
lone committer finishes on its own thread, and concurrent sync commits
still share one fsync.
A lone commit is published on its own thread, 1,600 concurrent commits
come out in submission order sharing barriers, and a failed barrier
fails every commit after it. The first test fails on the old committer.
…ns.md

The group-commit section still said the feed is appended before the
generation becomes current, which #49 reversed.
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.

1 participant