-
Notifications
You must be signed in to change notification settings - Fork 631
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
DicomServer performance regression #1776
Comments
I figured out two reasons. First in DicomServer.ListForconnectionsAsync there is one loopt, that accepts new associations, creates a Scp instance, notifies that a service has started and then waits for the next connection.
asynchronously writes a value into a channel and blocks the loop and therefor prevents from new clients to connect. Additionally, the async method waited until the thread, which started the WriteAsync method, is available again. So the first step was to add ".ConfigureAwait(false)" to this call, which already improved a little bit. |
But then finally I realized, that this notification is only important for the cleanup loop, but it is not necessary at all, to await it. Therefore I called it with fire-and-forget. Everything seemed to work fine, and the performance became significant better, but still not as good as it was before. |
The other thing I noticed was, that the client strems where closed some time later. Sectra starts its next clients, after the previous were closed.
So it often takes longer until the servicesToDispose list is iteated and the services are actually disposed. Additionally, the Channel behaves different than the previous AsyncManualResetEvent. But with the Cannel it is different. Here in this endless loop the client Tasks as well as a Task that reads from the channel is started. if then a client-task winns this race and a cleanup is stratedn, but while that a new client connects, then the started Channelreader takes this entry from the channel. So after the the cleanup there is a new connected client, but the channel is empty because of this Task that was started. |
But while it was not so important, to remove the clients from the |
After that change the performance regression was gone. Please review the PR. |
Thanks for the elaborate investigation and explanation! Unfortunately we are still migrating our high traffic ingestion code base away from our v4 fork, so it is hard for me to detect these issues until then. (I can tell you what we do stress the DicomFile reader and writer A LOT already) It would be very nice if we could use the benchmarks to detect these performance regressions, but I'm not sure how that would work yet. Until then, I'll review your work as soon as I get the chance. Your explanations sound very reasonable at first sight. |
Describe the bug
Since the commit a6e0d0a the performance of DicomServer has a regression, which has impact on high-performance system.
We have a DicomServer running where there approx 8000 Associations per hour in average (Sectra sends each series in a separate association). After updating to a version newer than August 2023, the performance decreases dramatically.
To Reproduce
Start a server on a high-load environment :-)
Environment
Fellow Oak DICOM version: 5.1.2
OS: Windows Server 2019
Platform: .net core 6
The text was updated successfully, but these errors were encountered: