Hello all,
My last two posts on this were too long and got no replies, that's on
me. Here's a short(er) version, or at least that was my intention. :)
Patch set v6 (rebased on master today 9f4bd91a196):
0001. Add an opt-in C11 <stdatomic.h> implementation. Off by default
(-Duse_stdatomic / --with-stdatomic = auto|yes|no, default "no"),
traditional code untouched. This is the only one I'm proposing.
0002. [NOT FOR MERGE] benchmark harness, for transparency and your use.
0003. [NOT FOR MERGE... yet.] delete the traditional implementation.
Included to scope that someday, and because with all three applied
the default flips to stdatomic so CI/the CF bot actually exercise
the new code. Filename ending in ".patch_" so as not to be applied
by the cf-bot so we test 0001+0002 only.
Builds and passes check-world on x86-64 (meson + autoconf), aarch64,
RISC-V, FreeBSD, and Windows-on-ARM/MSVC. No measurable regression on
x86-64, parity to slightly faster on aarch64 pgbench.
Three things I learned along the way:
* A C11 memory_order_relaxed load is not the same as our volatile load.
pg_atomic_read_u32/u64 are documented "no barrier semantics" so
relaxed looks right, but volatile also stops the *compiler* from
reordering, and callers lean on that. With relaxed reads a parallel
hash join lost a tuple on RISC-V (join_hash "extremely_skewed",
19999 instead of 20000, ~1 run in 4 on real rv64). seq_cst on the
read fixes it, relaxed + a compiler barrier does not.
I also tried the obvious narrower fix, leave the read relaxed and put
a pg_read_barrier() in the one hash-join consumer. That did not work,
same loss rate. The Barrier/condition-variable/LWLock paths read via
pg_atomic_read_*() too and were relying on the same implicit ordering.
So on the C11 path it has to live in the primitive.
To be clear this is not a live bug in master, I built stock master on
the same box and join_hash passed 40/40. volatile + the address
dependency is enough today. seq_cst is just replacing something the
relaxed mapping throws away.
* Barriers need atomic_signal_fence() *and* atomic_thread_fence(). A
bare thread fence doesn't stop the compiler moving plain accesses,
which ours have to order.
* pg_atomic_flag has to be 32-bit, not a byte. RISC-V has no
byte-granular AMO so an 8-bit fetch_and becomes a RMW of the whole
word and clobbers neighbors in a packed slock_t. generic.h already
uses uint32 for this reason.
On performance, briefly: my first cut made read *and* write seq_cst and
cost 1.5-2% on aarch64 read-only pgbench. It was the store, seq_cst is
STLR on aarch64 and serializes writers on a hot line (1.7x-4.8x on a
contended-write microbench, 2-16 threads). Keeping the write relaxed
and only ordering the read erases it, aarch64 RO lands at 1.000-1.023 of
stock and x86-64 is within +/-0.5%.
What I'd like to know from my fellow Hackers:
1. Is an opt-in, default-off second implementation worth carrying at
all? Tom's concern was that this not become The Only Way, hence the
default. If the answer is no, that's fine and I'll stop pushing.
2. If yes, is seq_cst on the generic read acceptable? I'd love a way to
get it back to relaxed that survives RISC-V, I couldn't find one.
Ideas?
3. Heikki, is pg_atomic_bool still the direction? I'd rather rebase
onto it than carry my own flag type. And Nathan's recent
FastPathStrongRelationLocks conversion adds new pg_atomic_* callers
in lock.c, which is a decent argument for pinning down these
semantics no matter which implementation we keep.
MSVC caveat since it'll come up: needs VS 2022+ and
/experimental:c11atomics, which Microsoft still calls experimental.
Older MSVC just builds the traditional path. The configure/meson probe
is a link test with a -latomic fallback, a 64-bit compare-exchange can
lower to a libatomic call on some 32-bit targets and a compile-only
check would pass then fail to link.
best.
-greg
