-
Notifications
You must be signed in to change notification settings - Fork 493
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
Next round of ENHs for visual DICOM browser #1206
Conversation
02206e6
to
e689810
Compare
@lassoan I fixed the crashes issues. The issue was related to the logging design of having a DCMTK appender per each Job running in the workers threads. The crashes were reproducible only by setting DEBUG level to DCMTK (probably in INFO level, there was not much text/updates). The only solution was to opt for the one appender design: only one appender handled by the ctkDICOMScheduler operating in the main thread. Of course this means that in DICOM detailed logging mode (i.e. DCMTK Debug level), there is a lot of text parsing going on in the main thread. It is not a major perfomance drawback, but it is noticeable. I think it is fine since in default the DICOM detailed logging option is disabled, and it would be used only for debug purposes (for which would be very useful to have the DCMTK logging per job). Please feel free to test and review. Thanks! |
This sounds good! Dynamic creation and deletion of hundreds of log appenders was not how these appenders meant to be used; and removing a log event processor in a multi-threaded environment where any thread can emit log events at any time was a very risky thing to do anyway. So, I'm glad that we don't do this anymore. Slower speed due to detailed logging is acceptable, because as you said, detailed logging will only be used for troubleshooting. |
f9f040b
to
b2d5bb4
Compare
ENH: Set 'Clear Completed' button to clear also 'Attempt Failed' jobs ENH: Set job status to 'user-stopped' when user stops jobs ENH: Set job status to 'attempt-failed' or 'failed' based on retry condition ENH: Change filter settings to reduce log noise from failed attempts ENH: Open corresponding patient widget when a job list row is clicked ENH: Improve job details formatting: adjust separators and remove spaces before ':'
This enhancement streams the DCMTK logging for each individual job directly to the Job List User Interface. This improves visibility and accessibility of log information for each job, aiding in debugging and monitoring job performance.
This commit addresses an efficiency issue in the implementation of the Observer pattern within the ctkDICOMScheduler and its associated UI widgets. The system had the potential to emit a large number of signals: The ctkDICOMScheduler functions as an intermediary between the UI and the underlying logic, tunneling all processing signals from the logic through itself. The UI components monitor the ctkDICOMScheduler to react to these signals. This leads to an O(N^2) complexity problem when dealing with many patients/studies/series. To mitigate this, several strategies have been implemented: 1. **Batching and Throttling**: Changes are now batched together and a throttling mechanism has been introduced. This mechanism delays the processing of changes, reducing the number of signals by waiting a certain amount of time since the last signal before sending a new one. This is particularly effective when changes often occur in bursts. 2. **Filtering**: A filtering mechanism has been added to the signals, allowing only relevant changes to be signaled to each observer. This is achieved by adding a parameter to the signals that specifies the type of changes the signal represents. 3. **Hierarchical Observers**: The hierarchical relationship of the observers has been leveraged to reduce the number of signals. Now, each object observes its nearest ancestor that has changed, rather than observing the ctkDICOMScheduler directly.
baf0f7b
to
04d1f55
Compare
4483989
to
7dca0f3
Compare
2f383af
to
5255c53
Compare
923e8c2
to
967de75
Compare
c015e3a
to
3e2919c
Compare
… series widget destruction. PERF: Disable SEG rendering in thumbnails due to limited support and potential performance issues. Replace with a blank thumbnail featuring a document SVG. BUG: Resolve issue hanging job termination when status is 'Queued'. BUG: Address issue with FreezeJobsScheduling during shutdown. BUG: Correct retry functionality when status icon on series widget is clicked.
3e2919c
to
f59aef0
Compare
38fbfdb
to
6beda5f
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.
Thank you, I've tested it and it works well!
Ref: #1162