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

(DO NOT MERGE) Remove semaphore that serializes access to Add/Push/PushUntrusted in MessagePool #10783

Closed
wants to merge 1 commit into from

Conversation

fridrik01
Copy link
Contributor

@fridrik01 fridrik01 commented Apr 28, 2023

PROPOSAL ONLY - Do not merge

This PR removes a semaphore that actually serialized all access to the Add(), Push() and PushUntrusted() methods in chain/messagepool so that only one goroutine thread can run it at once.

This semaphore may have be contributing to the high latency spikes we see on the glif nodes

These methods already have a mutex guarding critical sections, but the question is whether its needed when publishing message to topics (which I guess can take a while since it calls out to libp2p and other peers).

This commit removes a semaphore that serializes access to Add(),
Push() and PushLimited() so that only one goroutine thread can
run it at once.

These methods already have a mutex guarding critical sections,
but the question is whether its needed when publishing message
to topics (which I guess can take a while since it calls out
to libp2p and other peers).
@fridrik01 fridrik01 marked this pull request as ready for review April 28, 2023 10:58
@fridrik01 fridrik01 requested a review from a team as a code owner April 28, 2023 10:58
@fridrik01 fridrik01 changed the title (DO NOT MERGE) Remove semaphore that serializes access to Add/Push/PushLimited in MessagePool (DO NOT MERGE) Remove semaphore that serializes access to Add/Push/PushUntrusted in MessagePool Apr 28, 2023
@fridrik01
Copy link
Contributor Author

Talked about this with Steb, and this semaphore may be required to prevent exploitation of overloading the messagequeue, however the comment in the code for preventing lock contention should be changed

@fridrik01 fridrik01 closed this May 4, 2023
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.

1 participant