Skip to content

Support seqlock and use it for TaskGroup CPU time stat - #3541

Open
chenBright wants to merge 1 commit into
apache:masterfrom
chenBright:seqlock
Open

Support seqlock and use it for TaskGroup CPU time stat#3541
chenBright wants to merge 1 commit into
apache:masterfrom
chenBright:seqlock

Conversation

@chenBright

Copy link
Copy Markdown
Contributor

What problem does this PR solve?

Issue Number: resolve

Problem Summary:

TaskGroup tracks per-group CPU time as a 128-bit stat (a packed
last_run_ns + task-type word and a cumulated_cputime_ns word) that a
worker updates while other threads (e.g. bvar sampling) read it
concurrently. This was implemented with AtomicInteger128, whose 128-bit
"atomic" load/store had no portable, guaranteed lock-free backing:

  • On x86 it relied on wide SSE/AVX aligned loads/stores being atomic. This is
    not guaranteed by the ISA, because Intel and AMD do not officially promise 128-bit
    AVX load/store atomicity. It merely happens to hold on current microarchitectures
    (Skylake, Zen 2). Depending on it is relying on unspecified hardware behavior.
  • On platforms without such a wide atomic it fell back to a mutex, which serializes
    readers and blocks them behind the writer. Exactly what a consistent-snapshot
    read is meant to avoid.

What is changed and the side effects?

Changed:

  • Add butil::Seqlock, a general sequence lock providing lock-free, consistent snapshot
    reads around a caller-owned atomic payload.
  • Replace TaskGroup::AtomicInteger128 with an AtomicCPUTimeStat backed by
    butil::Seqlock<> over a CPUTimeStat payload accessed via relaxed atomics.

Side effects:

  • Performance effects:

  • Breaking backward compatibility:


Check List:

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Critical correctness and portability findings remain, along with moderate synchronization, test-coverage, and build-configuration findings.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

This PR adds a reusable seqlock and applies it to TaskGroup CPU-time statistics, replacing the previous 128-bit mechanism.

Changes:

  • Adds seqlock and shared processor helpers.
  • Updates TaskGroup CPU-stat synchronization.
  • Adds tests and Make/CMake/Bazel integration.
  • Changes Bazel’s default compilation mode.
File summaries
File Reviewed change Final review notes
test/seqlock_unittest.cpp Adds functional, concurrency, and performance tests. No final comment.
test/Makefile Registers the new test. No final comment.
test/CMakeLists.txt Registers the new test. No final comment.
test/BUILD.bazel Registers the new test with Bazel. No final comment.
src/butil/synchronization/seqlock.h Implements seqlock synchronization. Moderate, 3 votes: uint64_t is not guaranteed lock-free on supported targets, potentially reintroducing serialization.
src/butil/processor.h Provides shared processor helpers. Moderate, 1 vote: Unprefixed macros can break consumer code. Critical, 1 vote: The MSVC path uses unsupported GCC inline assembly.
src/bthread/task_group.h Integrates seqlock-backed CPU statistics. Critical, 2 votes: Type-punning plain integers as atomics is not valid. Moderate, 1 vote: Tests do not cover concurrent TaskGroup CPU-stat integration.
src/bthread/task_group.cpp Removes the old 128-bit implementation. No final comment.
src/bthread/processor.h Reuses shared processor helpers. No final comment.
.bazelrc Changes Bazel’s default compilation mode. Moderate, 3 votes: The global optimized mode should be scoped to the performance test.
Review details

Suppressed comments (2)

src/bthread/task_group.h:281

  • The added tests exercise only a standalone payload; none drives AtomicCPUTimeStat/TaskGroup::cumulated_cputime_ns() while sched_to publishes stats and the sampler reads them. The production-specific storage, task-type bit, and elapsed-time integration can therefore regress while all of these tests pass. Add a focused TaskGroup CPU-stat concurrency test.
            return _seqlock.load([&]() -> CPUTimeStat {
                return _stat.atomic_load();
            });

src/butil/processor.h:45

  • Because seqlock.h includes this header, every user of the new public butil::Seqlock now receives the unprefixed barrier() macro (and cpu_relax() above). A normal user declaration or call named barrier can therefore be rewritten by the preprocessor, creating an unrelated compile break. Keep these helpers private or prefixed rather than exporting the bthread implementation macros from a public butil header.
#ifndef barrier
#define barrier() asm volatile("": : :"memory")
  • Files reviewed: 10/10 changed files
  • Comments generated: 4
  • Review effort level: Lite

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment thread src/bthread/task_group.h Outdated
Comment thread src/butil/processor.h
Comment thread .bazelrc Outdated
Comment thread src/butil/synchronization/seqlock.h

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

Address the MIPS portability issue, remove the global Bazel optimization setting, and add focused CPU-stat tests.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (2)

.bazelrc:33

  • This unconditional option changes every default Bazel build and test from the repository's documented fastbuild mode (see the comment at lines 37-39) to opt, so bazel test now compiles the whole suite with optimization/NDEBUG and no longer exercises debug-only checks. Please remove this repository-wide setting; if the performance test needs optimization, scope it to the relevant command or target instead.
build -c opt

src/bthread/task_group.h:277

  • The added tests exercise a generic Payload, but none instantiate CPUTimeStat/AtomicCPUTimeStat or drive the TaskGroup writer and sampling paths changed here. A regression in the custom relaxed-atomic copy, bit packing, or load_for_writer() integration could therefore pass all of these tests; add a focused TaskGroup CPU-time-stat test covering writer updates and concurrent snapshot reads.
            return _seqlock.load([&]() -> CPUTimeStat {
                return _stat;
            });
  • Files reviewed: 10/10 changed files
  • Comments generated: 1
  • Review effort level: Lite

Comment thread src/butil/processor.h
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.

2 participants