Copilot commented on code in PR #3541: URL: https://github.com/apache/brpc/pull/3541#discussion_r3999121353
########## .bazelrc: ########## @@ -30,6 +30,7 @@ common --registry=https://baidu.github.io/babylon/registry common --registry=https://raw.githubusercontent.com/apache/brpc/master/registry build --verbose_failures +build -c opt Review Comment: This global setting changes the default compilation mode for every Bazel target and test, not just the new performance test. `bazel test` will now run optimized code instead of the prior default, changing debug-check behavior and potentially hiding debug-only failures in unrelated tests. Keep the repository default unchanged and pass optimized mode only where the benchmark requires it. ########## src/butil/synchronization/seqlock.h: ########## @@ -0,0 +1,302 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +#ifndef BUTIL_SYNCHRONIZATION_SEQLOCK_H +#define BUTIL_SYNCHRONIZATION_SEQLOCK_H + +#include <stdint.h> +#include <functional> +#include <mutex> +#include <type_traits> +#include <utility> + +#include "butil/atomicops.h" +#include "butil/compiler_specific.h" +#include "butil/macros.h" +#include "butil/processor.h" + +namespace butil { +namespace internal { + +// Detects std::reference_wrapper<T>. +template <typename T> +struct IsReferenceWrapper : std::false_type {}; +template <typename T> +struct IsReferenceWrapper<std::reference_wrapper<T>> : std::true_type {}; + +// A sequence counter for consistent reads without acquiring a reader mutex +// around caller-owned payloads. This is an implementation detail of butil::Seqlock; +// use butil::Seqlock instead. +// +// SeqCounter does not protect payload accesses from C++ data races. Callers must +// access shared payloads atomically (typically with relaxed ordering) or use +// another mechanism that makes concurrent accesses valid. +// +// +// Cache-line aligned so the sequence counter does not falsely share a line +// with adjacent data (e.g. the writer mutex in Seqlock<Mutex> or a neighbouring +// payload), which would bounce the line between readers and writers. +class BAIDU_CACHELINE_ALIGNMENT SeqCounter { +public: + SeqCounter() : _seq(0) {} + DISALLOW_COPY_AND_ASSIGN(SeqCounter); + + // Repeatedly invoke `load_payload` until it observes one consistent version. + // `load_payload` may run several times when it races with a writer, so it + // must be cheap and side-effect free: only read the shared payload and + // return a copy, never mutate observable state. It must also access the + // shared payload without causing a C++ data race (relaxed atomics). + // + // The callback MUST return an owning value (a copy of the data), never a + // pointer, reference, or view (string_view, span, reference_wrapper, ...) + // into the payload. The consistency guarantee covers only the bytes copied + // out before the validating load: once load() returns, a later writer may + // mutate the payload, so any handle that still points into it no longer + // refers to a consistent snapshot. + template <typename Load> + typename std::decay<decltype(std::declval<Load&>()())>::type + load(Load&& load_payload) const { + typedef typename std::decay<decltype(std::declval<Load&>()())>::type Result; + static_assert(!std::is_void<Result>::value, + "SeqCounter load callback must return a value"); + static_assert(!std::is_pointer<Result>::value, + "SeqCounter load callback must return an owning value, not " + "a pointer into the payload: the pointee can be mutated by " + "a later writer, so it is not a consistent snapshot"); + static_assert(!IsReferenceWrapper<Result>::value, + "SeqCounter load callback must return an owning value, not " + "a std::reference_wrapper into the payload: the referent " + "can be mutated by a later writer, so it is not a " + "consistent snapshot"); + + while (true) { + // Wait for any active writer, then read on an even sequence. + uint64_t seq; + while ((seq = _seq.load(memory_order_acquire)) & 1) { + cpu_relax(); + } + + Result result = load_payload(); + + // Keep the payload reads above before the sequence validation + // below; retry if a writer intervened. + atomic_thread_fence(memory_order_acquire); + if (_seq.load(memory_order_relaxed) == seq) { + return result; + } + } + } + + // Invoke a payload writer inside one write section. Writers must be serialized + // externally and must not nest write sections. The section is closed via RAII + // even if `store_payload` throws, so an exception does not leave the sequence + // permanently odd. A throwing writer may leave the payload partially updated, + // so callers must ensure any partial state is still safe (non-crashing) to read. + template <typename Store> + void store(Store&& store_payload) { + WriteGuard guard(*this); + store_payload(); + } + +private: + // RAII scope for a write section. + class WriteGuard { + public: + explicit WriteGuard(SeqCounter& sc) + : _seq_counter(&sc) + , _next_seq(sc._seq.load(memory_order_relaxed) + 2) { + // Writers are serialized, so no atomic read-modify-write is needed. + // Keep the next even sequence for publication when the guard exits. + _seq_counter->_seq.store(_next_seq - 1, memory_order_relaxed); + // Order the odd-sequence store before the payload stores that + // follow. A release fence is a StoreStore (+LoadStore) barrier: it + // keeps any store after the fence (the payload writes) from being + // reordered ahead of stores before it (the odd sequence), so no + // reader can observe a payload write without also observing the odd + // sequence. Equivalently, this release fence pairs -- through the + // payload atomics -- with the acquire fence in load(): if a reader + // reads a payload value published after this fence, that fence + // synchronizes-with the reader's acquire fence, so the reader is + // guaranteed to also see the odd sequence on its validating load + // and retry. The acquire half of an acq_rel fence would add + // nothing here (no prior load needs ordering), so release alone is + // sufficient and states the intent precisely. + atomic_thread_fence(memory_order_release); + } + + DISALLOW_COPY_AND_ASSIGN(WriteGuard); + + ~WriteGuard() { + // Publish the payload stores and leave the write section. + _seq_counter->_seq.store(_next_seq, memory_order_release); + } + + private: + SeqCounter* _seq_counter; + uint64_t _next_seq; + }; + + atomic<uint64_t> _seq; Review Comment: `butil::atomic<uint64_t>` is not required to be lock-free by C++, and this repository supports 32-bit targets where 64-bit atomic operations may be implemented with a hidden lock. Consequently this counter can reintroduce the reader/writer serialization this change is intended to remove. Use a width with a documented lock-free implementation on all supported targets (and define its wraparound policy), or explicitly constrain the API/platforms instead. ########## src/bthread/task_group.h: ########## @@ -259,6 +226,10 @@ friend class TaskControl; int64_t last_run_ns_and_type() const { return _last_run_ns_and_type; } + int64_t last_run_ns_and_type_atomic_load() const { + return ((butil::atomic<int64_t>*)&_last_run_ns_and_type) + ->load(butil::memory_order_relaxed); Review Comment: These casts do not make the `int64_t` members atomic: they invoke `butil::atomic<int64_t>` methods on storage whose object is actually a plain `int64_t` (and `butil::atomic` is a wrapper class derived from `std::atomic`). That is outside the C++ object/type and atomic-access guarantees, so under the existing strict-aliasing builds the seqlock payload can still be undefined behavior or be miscompiled. Keep actual atomic members in the shared payload (with a separate value-only `CPUTimeStat` if needed) instead of type-punning these fields. ########## src/butil/processor.h: ########## @@ -0,0 +1,48 @@ +// Licensed to the Apache Software Foundation (ASF) under one +// or more contributor license agreements. See the NOTICE file +// distributed with this work for additional information +// regarding copyright ownership. The ASF licenses this file +// to you under the Apache License, Version 2.0 (the +// "License"); you may not use this file except in compliance +// with the License. You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, +// software distributed under the License is distributed on an +// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +// KIND, either express or implied. See the License for the +// specific language governing permissions and limitations +// under the License. + +#ifndef BUTIL_PROCESSOR_H +#define BUTIL_PROCESSOR_H + +#include "butil/build_config.h" + +// Pause instruction to prevent excess processor bus usage, only works in GCC +#ifndef cpu_relax +#if defined(ARCH_CPU_ARM_FAMILY) +#define cpu_relax() asm volatile("yield\n": : :"memory") +#elif defined(ARCH_CPU_RISCV_FAMILY) +// Use the pause hint (Zihintpause extension). Encoding 0x0100000F +// (fence 0, 1) is a HINT on all RISC-V implementations: it never traps +// and is ignored on CPUs without Zihintpause. On CPUs with Zihintpause +// it provides a multi-cycle stall hint that reduces power and improves +// resource fairness during spin-wait loops. Matches the Linux kernel's +// RISC-V cpu_relax() behavior. .word is used instead of .insn or the +// pause mnemonic for maximum assembler compatibility. +# define cpu_relax() asm volatile(".word 0x0100000f\n": : :"memory") +#elif defined(ARCH_CPU_LOONGARCH64_FAMILY) +# define cpu_relax() asm volatile("nop\n": : :"memory"); +#else +# define cpu_relax() asm volatile("pause\n": : :"memory") +#endif Review Comment: On the supported MSVC path this fallback expands to GCC inline assembly. Because `SeqCounter::load()` calls `cpu_relax()` in the template body, including `seqlock.h` is a compile error with MSVC (x64 has no inline `asm`), so the new portable butil API cannot be used on Windows. Add a compiler-specific pause implementation such as `_mm_pause()`/`YieldProcessor()` before the generic inline-assembly fallback. -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
