Skip to content

feat(storage): separate read and hedging thread pools - #16389

Draft
ajayky-os wants to merge 1 commit into
googleapis:mainfrom
ajayky-os:perf/hedging_thread_pool
Draft

feat(storage): separate read and hedging thread pools#16389
ajayky-os wants to merge 1 commit into
googleapis:mainfrom
ajayky-os:perf/hedging_thread_pool

Conversation

@ajayky-os

Copy link
Copy Markdown
Contributor

This PR separates the thread pools used for primary reads and speculative hedged reads in Cloud Storage read hedging.

Previously, both primary reads and speculative hedges shared a single thread pool. This introduced two architectural issues:

  1. Resource Contention / Head-of-Line Blocking: When multiple primary reads stalled or ran concurrently, the shared pool could become saturated. Consequently, speculative hedge
    attempts were blocked from executing, defeating the primary purpose of hedging (tail latency mitigation).
  2. Asymmetric Sizing Needs: Primary reads block synchronously waiting on network I/O and require a higher concurrency ceiling (typically ≥64 threads or 4 × hardware concurrency),
    whereas speculative hedges are gated by rate limits and concurrency controls (typically bounded by MaxConcurrentHedgesOption or 2 × hardware concurrency).

This change introduces a general-purpose, lazily-scaling hedging_thread_pool.h:49 and composes it within hedging_thread_pool.h:145, isolating primary read execution from speculative hedges.

Key Changes

1. Dedicated ThreadPool and Embedded HedgingThreadPool

hedging_thread_pool.h:49:
• Dynamically scales workers on demand up to max_threads.
• Workers wait on a condition variable when idle and exit gracefully on shutdown.
• Automatically clamps max_threads to ≥1 to prevent deadlock/infinite hang if configured with 0.
• Supports self-destruction from within a worker thread (safely detaches rather than joining itself).

hedging_thread_pool.h:145:
• Embeds hedging_thread_pool.h:235 by value as its execution backend (declared last to guarantee worker joining before state teardown).
• Enforces the token-bucket rate limiter (ReadHedgeRateLimitOption) and maximum concurrent hedge ceiling (MaxConcurrentHedgesOption).

2. Dual Pool Configuration & Sizing Options

• Added options.h:122: Defaults to DefaultReadThreadPoolSize() (max (64,4 × cores)).
• Added options.h:135: Defaults to DefaultHedgingThreadPoolSize() (MaxConcurrentHedgesOption if set, else max (16,2 × cores)).
• Centralized sizing defaults in DefaultReadThreadPoolSize() and DefaultHedgingThreadPoolSize() so client.cc:604 and connection_impl.cc:165 remain consistent.

3. Isolation in HedgedObjectReadSource

• Updated hedged_object_read_source.cc:90 to accept separate read_pool_ and hedge_pool_.
• Primary attempt opens are scheduled onto read_pool_.
• Speculative hedged attempts are scheduled onto hedge_pool_.

@product-auto-label product-auto-label Bot added the api: storage Issues related to the Cloud Storage API. label Aug 27, 2026

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review

This pull request refactors the storage client's thread pool management by introducing a dedicated ThreadPool for primary read attempts, distinct from the HedgingThreadPool used for speculative hedges. It adds configuration options for thread pool sizes and updates the connection implementation, read source, and tests accordingly. The review feedback identifies a namespace compilation error in client.cc, requests the use of explicit types instead of auto for primitives in connection_impl.cc to comply with the style guide, and suggests caching thread pool size calculations in hedging_thread_pool.h for better performance.

Comment thread google/cloud/storage/client.cc
Comment thread google/cloud/storage/internal/connection_impl.cc Outdated
Comment thread google/cloud/storage/internal/connection_impl.cc Outdated
Comment thread google/cloud/storage/internal/hedging_thread_pool.h
@ajayky-os
ajayky-os force-pushed the perf/hedging_thread_pool branch from c4ac18d to 40cf649 Compare August 27, 2026 10:47
@codecov

codecov Bot commented Aug 27, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 93.57798% with 14 lines in your changes missing coverage. Please review.
✅ Project coverage is 92.30%. Comparing base (82fcb08) to head (40cf649).

Files with missing lines Patch % Lines
google/cloud/storage/internal/connection_impl.cc 8.33% 11 Missing ⚠️
...storage/internal/hedged_object_read_source_test.cc 97.72% 2 Missing ⚠️
...loud/storage/internal/hedged_object_read_source.cc 83.33% 1 Missing ⚠️
Additional details and impacted files
@@           Coverage Diff            @@
##             main   #16389    +/-   ##
========================================
  Coverage   92.30%   92.30%            
========================================
  Files        2245     2245            
  Lines      211870   211995   +125     
========================================
+ Hits       195568   195689   +121     
- Misses      16302    16306     +4     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

- Extract lazy, dynamically scaling ThreadPool primitive from HedgingThreadPool.
- Separate StorageConnectionImpl thread pool into a dedicated ReadThreadPool (for primary stream opens) and a HedgingThreadPool (for speculative secondary hedges).
- Add ReadThreadPoolSizeOption and HedgingThreadPoolSizeOption with auto-scaling defaults to prevent read bottlenecking under high concurrency.
- Extract DefaultReadThreadPoolSize() and DefaultHedgingThreadPoolSize() helpers to share sizing logic between DefaultOptions() and connection initialization.
- Enqueue primary read attempt to ReadThreadPool and speculative hedge attempts to HedgingThreadPool, ensuring complete fault and stall isolation.
- Clamp ThreadPool capacity to at least 1 to prevent deadlock on zero sizing.
- Add unit tests verifying thread pool execution, default sizes, zero-size handling, lazy spawning, and pool isolation under saturation.
@ajayky-os
ajayky-os force-pushed the perf/hedging_thread_pool branch from 40cf649 to 9a014d7 Compare August 27, 2026 11:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

api: storage Issues related to the Cloud Storage API.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant