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

Improve handling of multiple JEventSources #176

Merged
merged 5 commits into from
Nov 22, 2022
Merged

Conversation

nathanwbrei
Copy link
Collaborator

Previously, multiple event sources would each be assigned separate JEventSourceArrows and be run at the same time. This PR changes JEventProcessorArrow so that it accepts a vector of JEventSources and runs them one after another until each completes or reaches the max jana:nevents'. This leads to fewer files/sockets being open simultaneously, better performance due to fewer calls to JFactory::ChangeRun` etc, less work for the scheduler, and a more easy-to-reason-about event stream. The old behavior is easy to bring back in the JTopologyBuilder if we ever want it.

This addresses issue #146.

It includes some refactorings:

  • JEventSource::DoNext now handles all of the logic for setting up a JEvent prior to JEventSource::GetEvent. Previously some of this was mixed in with JEventSourceArrow.
  • JEventSource::DoNext() now calls JEventSource::Close() when JEventSource::GetEvent() declares itself finished.
  • JEventSource::SetRange() is replaced with JEventSource::SetNEvents and SetNSkip. The corresponding getters have been added.

There is one small behavior modification:

  • JEventSource::Open() is now always called from JEventSourceArrow::execute(), whereas before it was called from JEventSourceArrow::initialize(). This changes the behavior of JApplication::Quit() when called from inside JEventSource::Open, as well as Ctrl-C: Previously, no events would be emitted and Run() would return before even firing up the workers. Now, Quit() will trigger a graceful shutdown but won't immediately stop the source from emitting JEvents. This is less useful but more consistent and correct: if you want to handle an error condition in JEventSource::Open(), you should throw a JException instead.

@faustus123
Copy link
Collaborator

Do I understand correctly that this will not call finalize on any JEventSource until the end of processing? I guess more to the point, will the source files all be kept open until the end of the program vs. closing them earlier? I ask since we have seen where people may run jan giving it a hundred or more small skim files in one job and running into issues with having that many open file descriptors.

@nathanwbrei
Copy link
Collaborator Author

@faustus123 Nope, JEventSources are closed as soon as possible, in other words, in DoNext(). The only time that a JEventSource gets closed during finalize() is when the user Ctrl-C'ed or called JApplication::Quit().

@nathanwbrei nathanwbrei merged commit d9153fd into master Nov 22, 2022
@cissieAB cissieAB deleted the nbrei_issue_146 branch July 20, 2023 07:12
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