perf(engine): replace assignedCountMu with atomic.Int64#4189
perf(engine): replace assignedCountMu with atomic.Int64#4189salarkhannn wants to merge 6 commits into
Conversation
|
@salarkhannn is attempting to deploy a commit to the Hatchet Team on Vercel. A member of the Team first needs to authorize it. |
gregfurman
left a comment
gregfurman
left a comment
There was a problem hiding this comment.
Hey @salarkhannn. I'm not certain you've read our contributing guidelines.
We require that a PR is:
- Linted and formatted.
- titled + scoped correctly following conventional commits format.
- AI usage disclosed.
For small changes like this, we're normally happy not requiring a linked issue. However, since this purports to improve performance, I'd like to see some more evidence before proceeding with a review.
Can you rework this PR to adhere to our guidelines and attach these benchmarks as additional information in the ## Related section following our https://github.com/hatchet-dev/hatchet/blob/main/.github/pull_request_template.md?
salarkhannn
changed the title
|
I'm sorry i didn't look at the contributing guidelines earlier. if there are any discrepancies, lmk |
|
@salarkhannn Can you please rebase with current |
assignedCount is only ever incremented and read under a single mutex critical section. Replace the int+mutex pair with atomic.Int64 so the increment-and-read sequence is a single Add(1) call with no lock contention, eliminating one mutex acquisition per task assignment on the scheduler hot path.
salarkhannn
force-pushed
the
perf-atomic-assignedcount
branch
from
d4637e9 to
a10363c
Compare
3 participants
Description
Replaces assignedCount (int + sync.Mutex) with atomic.Int64 so the increment and read sequence is a single Add(1) call with no lock contention. This eliminates one mutex acquisition per task assignment on the scheduler hot path.
Type of change
What's Changed
Checklist
Changes have been:
Related
goos: linux
goarch: amd64
cpu: 12th Gen Intel(R) Core(TM) i5-12450H
AssignedCountMutex-12 40.71m ± 10%
AssignedCountAtomic-12 33.79m ± 13%
The atomic version is 17% faster (p=0.002, n=6). The block profile also confirms assignedCountMu.Lock() is eliminated after the change. All existing tests pass with the race detector enabled.
AI Disclosure
I acknowledge that an LLM was used in the creation of this Pull Request, in accordance with Hatchets AI_POLICY.md.
Details: used opencode for git workflows and formatting