Skip to content

621 configurable ingestion thread count#636

Open
fkengun wants to merge 13 commits intodevelopfrom
621-configurable-ingestion-thread-count
Open

621 configurable ingestion thread count#636
fkengun wants to merge 13 commits intodevelopfrom
621-configurable-ingestion-thread-count

Conversation

@fkengun
Copy link
Copy Markdown
Contributor

@fkengun fkengun commented May 6, 2026

Changes in this PR:

  • Add configs for ingestion thread count in ChronoKeeper, ChronoGrapher and ChronoPlayer. Default values are 4, 1 and 1, respectively.

@fkengun fkengun requested review from Copilot, iameneko and ibrodkin May 6, 2026 20:05
@fkengun fkengun self-assigned this May 6, 2026
@fkengun fkengun added the Performance Performance optimizations label May 6, 2026
@fkengun fkengun linked an issue May 6, 2026 that may be closed by this pull request
@github-actions

This comment was marked as outdated.

Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR adds configurable ingestion/RPC thread counts for ChronoKeeper, ChronoGrapher, and ChronoPlayer recording/query services via JSON configuration, and wires the configured values into the margo_init(..., rpc_thread_count) setup to control server-side RPC concurrency.

Changes:

  • Added IngestionThreadCount configuration under each service block and updated default_conf.json.in defaults (Keeper=4, Grapher=1, Player=1).
  • Refactored service configuration structs to nest RPC settings under RPC_CONF and updated call sites accordingly.
  • Updated server initialization to pass configured thread counts to margo_init, and adjusted an integration test to match the new config shape.

Reviewed changes

Copilot reviewed 14 out of 14 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
test/integration/keeper-grapher/keeper_grapher_ingest_test.cpp Updates test to use nested RPC_CONF fields for KeeperGrapherDrainService addressing/provider id.
default_conf.json.in Adds IngestionThreadCount keys and sets defaults per component.
ChronoPlayer/src/ChronoPlayerConfiguration.cpp Parses IngestionThreadCount and routes rpc parsing through RPC_CONF.
ChronoPlayer/src/ChronoPlayer.cpp Uses configured thread count when calling margo_init for PlaybackQueryService.
ChronoPlayer/include/ChronoPlayerConfiguration.h Introduces PlaybackServiceConf wrapper with RPC_CONF and thread count.
ChronoKeeper/src/ChronoKeeperInstance.cpp Uses configured thread count when calling margo_init for KeeperRecordingService.
ChronoKeeper/src/ChronoKeeperConfiguration.cpp Parses IngestionThreadCount under KeeperRecordingService and routes rpc parsing through RPC_CONF.
ChronoKeeper/include/KeeperRecordingService.h Adjusts per-event logging (TRACE in debug builds + added debug line for ULT/ES).
ChronoKeeper/include/IngestionQueue.h Tweaks logging level/guards inside ingestion path.
ChronoKeeper/include/ChronoKeeperConfiguration.h Introduces KeeperRecordingServiceConf wrapper with RPC_CONF and thread count.
ChronoGrapher/src/ChronoGrapherConfiguration.cpp Parses IngestionThreadCount under KeeperGrapherDrainService and routes rpc parsing through RPC_CONF.
ChronoGrapher/src/ChronoGrapher.cpp Uses configured thread count when calling margo_init for KeeperGrapherDrainService.
ChronoGrapher/include/ChronoGrapherConfiguration.h Introduces KeeperGrapherDrainServiceConf wrapper with RPC_CONF and thread count.
chrono_common/src/deprecated_ConfigurationManager.cpp Adds parsing for IngestionThreadCount (deprecated path).
Comments suppressed due to low confidence (1)

ChronoKeeper/include/IngestionQueue.h:73

  • ingestLogEvent() reads from storyIngestionHandles without holding ingestionQueueMutex (find/size/use). With this PR increasing Margo RPC thread counts (e.g., ChronoKeeper default 4), record_event can call ingestLogEvent concurrently, which makes unordered_map access a data race and can lead to crashes/corruption. Protect all accesses to storyIngestionHandles with a lock (or switch to a reader/writer strategy) and ensure the handle pointer remains valid while used.
#endif
        auto ingestionHandle_iter = storyIngestionHandles.find(event.storyId);
        if(ingestionHandle_iter == storyIngestionHandles.end())
        {
            LOG_WARNING("[IngestionQueue] Orphan event for story {}. Storing for later processing.", event.storyId);

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread ChronoKeeper/include/ChronoKeeperConfiguration.h
Comment thread ChronoKeeper/src/ChronoKeeperInstance.cpp
Comment thread ChronoGrapher/src/ChronoGrapher.cpp
Comment thread ChronoKeeper/include/KeeperRecordingService.h Outdated
Comment thread chrono_common/src/deprecated_ConfigurationManager.cpp Outdated
@fkengun fkengun marked this pull request as draft May 6, 2026 20:14
@fkengun fkengun marked this pull request as ready for review May 7, 2026 05:43
Comment thread ChronoKeeper/include/IngestionQueue.h Outdated
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
@iameneko iameneko self-requested a review May 7, 2026 15:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Performance Performance optimizations

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Configurable ingestion thread count

3 participants