Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Perf[BMQIO]: remove unnecessary function copy #483

Merged
merged 2 commits into from
Nov 4, 2024

Conversation

678098
Copy link
Collaborator

@678098 678098 commented Oct 28, 2024

IO thread should spend 7.5% (1.7 / 22.96) less cpu

Before:
Screenshot 2024-10-28 at 16 33 12

After:
Screenshot 2024-10-28 at 17 27 43

@678098 678098 requested a review from a team as a code owner October 28, 2024 16:43
@678098 678098 force-pushed the 241028_rm_function_copy branch 3 times, most recently from a512422 to 8b0c00f Compare October 28, 2024 18:12
@@ -758,11 +755,19 @@ void NtcChannel::processReadQueueLowWatermark(

int numNeeded = 0;
{
bmqio::Channel::ReadCallback readCallback = read->callback();
const bmqio::Channel::ReadCallback& readCallback =
read->callback();

bslmt::UnLockGuard<bslmt::Mutex> unlock(&d_mutex);
readCallback(bmqio::Status(), &numNeeded, &d_readCache);
Copy link
Collaborator Author

@678098 678098 Oct 28, 2024

Choose a reason for hiding this comment

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

    bslmt::UnLockGuard<bslmt::Mutex> unlock(&d_mutex);
    // !!!!!!!!! `close` might be called from another thread
    readCallback(bmqio::Status(), &numNeeded, &d_readCache);

Here, we have a non-trivial thread race. In the past, we could have emptied the d_callback of NtcRead between these 2 lines of code. We make a copy of d_callback before unlocking a mutex, so we don't access the empty callback. However, it means that we can send another callback from this copy after we executed close.

My PR doesn't fix this race, so we still can send something over this callback. Instead, I make sure not to clear it from another thread, so this reference is always alive. And I also added a post-check for isComplete() to return early.

@678098 678098 force-pushed the 241028_rm_function_copy branch from 8b0c00f to 699d567 Compare October 28, 2024 19:58
Copy link

@bmq-oss-ci bmq-oss-ci bot left a comment

Choose a reason for hiding this comment

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

Build 341 of commit 699d567 has completed with FAILURE

Copy link
Collaborator

@dorjesinpo dorjesinpo left a comment

Choose a reason for hiding this comment

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

Maybe, we can eliminate setComplete now and use clear instead?

@678098
Copy link
Collaborator Author

678098 commented Oct 30, 2024

Maybe, we can eliminate setComplete now and use clear instead?

Done. Made sure that we have clear down the code paths where I removed setComplete.

Copy link

@bmq-oss-ci bmq-oss-ci bot left a comment

Choose a reason for hiding this comment

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

Build 346 of commit f0fcea3 has completed with FAILURE

@678098 678098 requested a review from dorjesinpo October 31, 2024 19:06
@678098 678098 merged commit b909a17 into bloomberg:main Nov 4, 2024
34 of 35 checks passed
@678098 678098 deleted the 241028_rm_function_copy branch November 4, 2024 15:36
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