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

nsqd: optimize GetTopic / fix data race #709

Merged
merged 1 commit into from
Jan 16, 2016

Conversation

absolute8511
Copy link
Contributor

In AggregateChannelE2eProcessingLatency the channels should be copied out with lock and this lock should be outside the stream merge. Also the GetTopic is hot while pub message, so we should avoid write lock as more as possible.

@jehiah
Copy link
Member

jehiah commented Jan 15, 2016

Good catch on the race in AggregateChannelE2eProcessingLatency. Do you have more changes you want to make in this PR?

Can you add a bench test and share before/after metrics for performance change from the GetTopic() hot path?

@absolute8511
Copy link
Contributor Author

OK, I will add some benchmark test later.

@mreiferson mreiferson changed the title Optimize lock nsqd: optimize lock Jan 15, 2016
@mreiferson
Copy link
Member

also LGTM, I am interested in benchmarks but can you also squash down into two separate commits

@mreiferson
Copy link
Member

thanks for all these fixes @absolute8511

@mreiferson mreiferson changed the title nsqd: optimize lock nsqd: optimize GetTopic / fix data race Jan 15, 2016
2.try rlock first to reduce write lock
@mreiferson
Copy link
Member

👍

mreiferson added a commit that referenced this pull request Jan 16, 2016
nsqd: optimize GetTopic / fix data race
@mreiferson mreiferson merged commit 43aeb4a into nsqio:master Jan 16, 2016
@absolute8511 absolute8511 deleted the optimize-lock branch August 8, 2017 08:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants