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(gossip): improve packet verification #110

Merged
merged 4 commits into from
Mar 24, 2024
Merged

perf(gossip): improve packet verification #110

merged 4 commits into from
Mar 24, 2024

Conversation

0xNineteen
Copy link
Contributor

prev implementation was a large bottleneck which would spawn a thread per packet and wait sequentially before scheduling a new thread for the next packet (a lot of thread overhead) and waited for all threads to be completed before draining new packets

this fix

  • assigns packet batches to each thread (arraylist(packet) vs packet)
  • schedules them on any free task
  • and doesnt wait for all the tasks to complete before draining from the channel

@0xNineteen 0xNineteen marked this pull request as ready for review March 24, 2024 17:51
@ultd ultd merged commit c8fa17b into main Mar 24, 2024
2 checks passed
@ultd ultd deleted the 19/gossip-verify-fix branch March 24, 2024 20:36
dnut added a commit that referenced this pull request Mar 25, 2024
Previously, we used PACKETS_BER_BATCH threads to process messages, because one thread was dispatched for each individual packet. This was intended to process a full packet batch at one time, but it was very inefficient, likely due to context switching and cache misses, which caused significant bottlenecks.

#110 addressed this by changing the logic to process entire packet batches within a thread, instead of just single packets. Now, there is no particular reason for the number of threads to equal PACKETS_PER_BATCH.

But we're still using PACKETS_PER_BATCH for the number of threads. That's 64 threads. This *might* have been useful on machines with high core-counts when the large number of threads was necessary to process a single batch at once. But now that a single thread can more efficiently manage an entire batch, there is no reason to use PACKETS_PER_BATCH threads. A more reasonable number can be used instead.

I selected 4 so it can still benefit from parallelism, but won't suffer from as much context switching, and won't hog system resources during heavy load, since this risks compromising the performance of other important tasks.
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