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

Next round of ENHs for visual DICOM browser #1206

Merged
merged 16 commits into from
Jun 11, 2024

Conversation

Punzo
Copy link
Contributor

@Punzo Punzo commented May 20, 2024

Ref: #1162

@Punzo Punzo force-pushed the visualDICOMBrowserENH branch 8 times, most recently from 02206e6 to e689810 Compare May 23, 2024 12:44
@Punzo Punzo marked this pull request as ready for review May 23, 2024 12:56
@Punzo
Copy link
Contributor Author

Punzo commented May 23, 2024

@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!

@lassoan
Copy link
Member

lassoan commented May 23, 2024

The only solution was to opt for the one appender design: only one appender

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.

@Punzo Punzo force-pushed the visualDICOMBrowserENH branch 6 times, most recently from f9f040b to b2d5bb4 Compare May 24, 2024 00:07
Punzo added 10 commits June 3, 2024 09:53
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.
@Punzo Punzo force-pushed the visualDICOMBrowserENH branch 2 times, most recently from 923e8c2 to 967de75 Compare June 4, 2024 14:55
@Punzo Punzo force-pushed the visualDICOMBrowserENH branch 2 times, most recently from c015e3a to 3e2919c Compare June 6, 2024 16:29
… 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.
Copy link
Member

@lassoan lassoan left a 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!

@lassoan lassoan merged commit db10ff2 into commontk:master Jun 11, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants