Skip to content

Conversation

@tvaron3
Copy link
Member

@tvaron3 tvaron3 commented Nov 25, 2025

Description

Following two changes from suggestions from @analogrelay to read many/ query apis,

  • Slight performance suggestion for a separate PR: Store request charges in an array with workerCount slots. Each worker can update the request charge slot using it's worker index w without a lock. Sum up all the slots at the end, when all the workers are done.

  • As a refactoring for later, we can actually just collect all the results and call ProvideData once with a single batch. That's what it's designed for. You're right that it shouldn't be called simultaneously, but calling it once with a batch of items will also reduce CGo overhead.

Copilot AI review requested due to automatic review settings November 25, 2025 16:55
@tvaron3 tvaron3 requested a review from a team as a code owner November 25, 2025 16:55
Copilot finished reviewing on behalf of tvaron3 November 25, 2025 16:58
Copy link
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 implements two performance optimizations to the runEngineRequests function based on code review feedback:

  1. Lock-free charge tracking: Replaces mutex-protected charge accumulation with per-worker array slots, eliminating lock contention when workers record request charges.

  2. Batched result delivery: Changes from calling ProvideData for each individual result to collecting all results and calling ProvideData once with a single batch, reducing CGo overhead.

Key Changes

  • Removed the provider goroutine and chargeMu mutex
  • Added per-worker charges array indexed by worker ID for lock-free charge accumulation
  • Introduced a collector goroutine that gathers all QueryResult items into allResults
  • Modified control flow to call pipeline.ProvideData once after all workers complete
  • Changed function signature to use unnamed return values

Copy link
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.

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants