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

G3NetworkSender background serialization #98

Merged
merged 5 commits into from
Apr 17, 2023

Conversation

cnweaver
Copy link
Contributor

@cnweaver cnweaver commented Feb 2, 2023

Refactor G3NetworkSender so that it can handle larger rates of large frames, for which frame serialization overhead can dominate over transmission time. To do this, the option is provided to run some number of background threads responsible for serializing frames. The default is to use zero serializer threads, and to continue to serialize on the main thread, as has previously been the case. Because the access to the serialized blobs inside G3Frame is not currently thread safe, frames are not emitted from G3NetworkSender until their serialization is completed. The mechanism to coordinate this also adds some small overhead to the case of serializing on the main thread, which appears to be unimportant, but which could be avoided if there is a need to do so.

@cnweaver cnweaver self-assigned this Feb 2, 2023
@cnweaver cnweaver requested a review from nwhitehorn February 2, 2023 17:49
@cnweaver
Copy link
Contributor Author

cnweaver commented Feb 9, 2023

There's a potential deadlock in this code, so it should not be merged just yet. I'm working on fixing it. (Roughly the same failure mode existed in the existing version, but was exceedingly difficult to trigger; unfortunately, by adding complexity I increased expose to it.)

Copy link
Member

@arahlin arahlin left a comment

Choose a reason for hiding this comment

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

I think this is ready for review? Some minor nits below, otherwise looks good, assuming it works.

core/src/G3NetworkSender.cxx Show resolved Hide resolved
core/src/G3NetworkSender.cxx Outdated Show resolved Hide resolved
@cnweaver
Copy link
Contributor Author

Yes, I think this is ready for full review; the gotchas that I found during testing or which nwhitehorn warned about should now be fixed. I've given this a fair amount of testing myself, particularly in the multiple serialization thread configuration, and also some more artificial testing with serialization on the main thread, and I have observed no further issues. However, if there are any particular tests which seem like they should be tried, let me know and I'll see what I can do.

@cnweaver cnweaver merged commit 69b0185 into master Apr 17, 2023
@cnweaver cnweaver deleted the g3networksender-serialization-scalability branch April 17, 2023 17:28
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.

2 participants