Skip to content

Memory Issue Discussion

Matthew Archer edited this page Mar 18, 2021 · 1 revision

Originally written by Matthew Archer (@Fincap) 10/09/2020 on Redmine

What is the issue?

When the MainWindow class is closed, the attached memory is not freed from the heap and as such, when new patients are subsequently loaded in the same application session, the memory leaks and eventually crashes due to a segmentation fault (usually at the stage that three patients have been opened). Another related issue is that when the MainWindow is closed (by way of pressing the ‘Red X’ or ‘File->Close’), the program crashes again. We believe these two issues to be intrinsically linked.

What have we discovered?

Through trial and error, many theories as to what is causing the crash and how to fix it have manifested. One prevailing theory was that it was a fundamental flaw in the PyDicom library’s dcmread function, due to the crash logs all pointing to this function. This is most likely not the case, however, as it is a simpler explanation to state that because this function is the only function that is reading new data into memory, it is obvious that: if a memory leak is occurring, then it would occur when new data is introduced into memory.

Another theory was that it would be related to incorrect handling of multi-threading. This was quickly disproved by the identification of a segmentation fault occurring within the main thread.

As part of narrowing down the direct cause of the memory leak, we theorized whether it was a particular functionality introduced by the 2020 team that spurred on the issue. In order to test this we cloned the repository environment from the last commit made before we started contributing and ran the program. By doing this we managed to identify that the leak was a problem all the way back then.

Through rigorous analysis using various debugging tools we have identified the root cause of the issue being that the MainWindow being closed leaves a vast amount of data in memory, including the DICOM dataset objects which can be up to hundreds of megabytes in size each .

As it stands, many of the UIMainWindow components pass in the UIMainWindow class itself as a parameter and make modifications to the UI directly from there, which does not adhere to many of the principles of Object-Oriented Programming. We believe that this relationship between UIMainWindow and its components is extremely likely of being the culprit for this issue.

Where to go from here?

Many different attempts have been made to get the MainWindow class to deconstruct itself correctly, however none have been shown to solve the issue. Due to this, we have devised a few workarounds.

The first workaround is to have the “Open Patient” button forcefully restart the application (rather than just closing the MainWindow and opening the OpenPatientWindow again). This would free the memory totally. This method does however have the downside of being slightly slower. The workaround was actually tested and found to work for a brief time fairly successfully, however shortly after testing it inexplicably stopped working. This success has not yet been recreated.

The next potential workaround is to restructure the UIMainWindow class. We believe that because many of the components of the UIMainWindow are not QObjects but contain many QObjects within then, that they are not being referenced correctly by Python’s garbage collector and as such not being cleaned up properly. As such, a restructure of the UIMainWindow class would entail ensuring that all components of the class are correctly structured inheritors of QObject (e.g. refactoring the DVH class to be a QWidget that could theoretically be put into any QApplication provided the right dataset). This promotes a healthier Object-Oriented architecture for the project. The downside of this workaround is that it would likely delay the ability of the team to work on the other requirements by at least a couple of weeks and still not guarantee that it will solve the issue.

The final and least attractive workaround is to simply remove the ability to open more than one patient successively in one session.

References

https://doc.qt.io/qt-5/model-view-programming.html

https://docs.python.org/3.1/library/gc.html