phongn commented on PR #13170:
URL: https://github.com/apache/trafficserver/pull/13170#issuecomment-5348542026
Following up on my earlier review with a correction, a root-cause analysis
of the regression, and a prototype that removes it.
## Correction to my benchmark table
My earlier numbers were built with the `dev` preset, which is a
Debug/low-optimization build. At low optimization the `std::atomic` wrapper
layers in this PR do not inline, which unfairly penalized this PR's code
structure relative to master's macro/union code. Your Release methodology was
right and my contended numbers were wrong — apologies for the noise. Re-run
with `--preset release`, interleaved A/B/C (each binary alternated within each
rep, 3 reps, 20 samples; 1-thread means reproduce within 1%):
| Configuration | 1 thread | 6 threads |
|---|---|---|
| master, clang (128-bit head, lock-free) | 56.7 ms | 2.19 s |
| this PR, clang (128-bit head + pop locks) | **73.1 ms (+29%)** | **1.60 s
(−27%)** |
| prototype, clang (128-bit head, lock-free + `atomic_ref`) | **56.7 ms (=
master)** | 2.02 s |
| master, gcc (128-bit head, `__sync`, lock-free) | 76.7 ms | 2.58 s |
| this PR, gcc (packed-64 head + pop locks) | 47.2 ms | 1.44 s |
| prototype, gcc (packed-64 head, lock-free + `atomic_ref`) | 30.1 ms | 1.59
s |
So in Release, the locks genuinely **win under heavy contention** — your
instinct there was correct and my earlier claim of a contended regression was
an artifact. The real cost is on the **uncontended fast path**: +29% per op at
1 thread on the 128-bit tier.
## Why this explains the end-to-end regression
I tested three mechanisms in isolation (microbenchmark with a versioned
128-bit Treiber stack):
* **Mutex fixed cost, uncontended** — confirmed: the lock/unlock pair adds
~30-45% per op at 1 thread, matching the table above.
* **False sharing** (the `std::mutex` and the head share a cache line: mutex
occupies bytes 0-39, head sits at offset 48) — **exonerated**: padding them
onto separate lines changes nothing measurable.
* **Lock convoy under contention** — exonerated: the lock is *better* than
CAS retry storms when contended.
In production, ProxyAllocator thread caches absorb most freelist traffic and
ProtectedQueue operations rarely collide, so the fleet mostly runs the
*uncontended* path — paying the +29% tax on every op while rarely collecting
the contended win. That is consistent with the 5-10% end-to-end regression you
measured internally.
## Prototype: same UB fix, no locks
The #11640 bug is a *data race* on the speculative next-pointer read — the
version-tagged CAS has always correctly rejected stale values; the algorithm
does not need mutual exclusion. The prototype keeps everything else in this PR
(memcpy views, placement-new lifetime handling, alignment asserts, LSan
annotations) and:
* removes the three `lock_guard`s and the now-unused `std::mutex` members,
and
* converts every next-slot access (pop's speculative read,
push/free/bulkfree stores, popall's fixup walk, remove's reads/writes) to
relaxed `std::atomic_ref<void *>`, so the race is defined behavior.
Relaxed slot accesses compile to plain loads/stores; the release/acquire
edge stays on the head CAS (slot store *sequenced-before* the release CAS;
popper's acquire on the head *synchronizes-with* it). Two details worth noting
because they are easy to miss: the value-initializing placement-news (`new (p)
void *{}`) were themselves plain stores into raced slots, so the prototype uses
default-init (`new (p) void *`, no store) followed by an atomic store; and
`ink_atomiclist_remove`'s slot accesses are converted too (it still requires
the documented single-popper discipline, as it always has, but its accesses are
now defined).
**Branch:** https://github.com/phongn/trafficserver/commit/988a139a57
(stacked on this PR's head, cherry-pick friendly)
**Validation:** passes a 6-thread conservation/double-reachability stress
test repeatedly on both head configurations, in both Debug and Release;
performance is exactly master at 1 thread (56.7 ms) and ≈ master at 6 threads
on the 128-bit tier.
**Honest residuals:** the Debug-only races remain (`dummy_forced_read`,
which you already documented, and `DEADBEEF`'s plain fills). And for
`InkAtomicList` specifically, a stale popper can touch caller-owned memory
after the pop — the same invariant master has relied on for decades (ironclad
for the freelist, whose chunks are never unmapped; theoretical for arbitrary
atomiclist items). The locks close that fully; `atomic_ref` only removes the
UB. If you want maximum conservatism, a hybrid is coherent: lock-free freelist
(the measured hot path) + locked atomiclist pops.
Given your internal 5-10% end-to-end regression was likely the uncontended
mutex tax, the prototype should recover it — it would be great to see it
through your internal benchmarking. Also worth noting from the table: gcc's
packed-64 lock-free configuration (30 ms) is by far the fastest, so once the
LogObject version-width hazard from my earlier review is addressed,
deliberately selecting the packed head could be a legitimate follow-up
optimization rather than a probe accident.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
