-
Notifications
You must be signed in to change notification settings - Fork 139
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
Improve the integration of WalkerLog classes in batched drivers #5039
Conversation
e5e38c5
to
2534ad4
Compare
3f6120b
to
91e242d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Efficient improvement. The .release() into the legacy drivers is a nice trick. Think this just needs the noted. documentation addition.
@jtkrogel could you review? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just one addition requested below (logic check).
Overall, the change to a "capture" style for collectors
makes the WalkerLogManager function API ambiguous due to semi-ownership of the collectors. Functions accepting collectors or not seems like an arbitrary choice, potentially raising questions as to whether the collectors passed in to a member function should ever differ from the captured set (they shouldn't). Despite this, I don't think its a blocking issue.
writebuffer requires the same set of collectors as startRun. Since the code has be changed to capture the set of references at the startRun, writebuffer doesn't need to take the set again as argument any longer. |
Test this please |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good enough.
Proposed changes
Made the following changes.
What type(s) of changes does this code introduce?
Does this introduce a breaking change?
What systems has this change been tested on?
epyc-server
Checklist