Image load slowness regression

[fedorov]

https://viewer.imaging.datacommons.cancer.gov/viewer/1.3.6.1.4.1.14519.5.2.1.1706.4009.243406849348906151042089406192?seriesInstanceUID=1.3.6.1.4.1.14519.5.2.1.1706.4009.304054433668574201838767925472 mentioned in https://github.com/OHIF/Viewers/issues/2192 used to load in the past, but not anymore.

Related to this, can we discuss the options for testing approaches to identify this kind of regressions, or at least have a way to measure load of a series to facilitate manual testing?

[pieper]

This is another scaling issue, probably due to the large number of requests. Several of the frames have very long load times:

[fedorov]

adding @wlongabaugh

[Punzo]

I did some debugging and I could reproduce that when scrolling the image the function VTKMPRToolbarButton (and therefore _isDisplaySetReconstructable) is called in an almost infinite loop.

Since in _isDisplaySetReconstructable we do "heavy" checks (i.e.: https://github.com/OHIF/Viewers/commit/b59187c74dfd91fc886ef0314809b097c5f84006 ) the interface gets stucked.

We could find better heuristics for the checks in _isDisplaySetReconstructable (but they are actually pretty fast even for datasets of 512x512x3000 voxels, < 1 sec), but the real issue is in the almost infinte calls loop.

@igoroctaviano do you have any idea why VTKMPRToolbarButton is called so many times (at every scroll)? and why goes in loop at each scroll? should it just be called once at initialization?

[Punzo]

I think having a test for checking the viewer loading time will be difficult: the reproducibility can be low and produce easily false negatives, since the general loading of the viewer can heavily depends on the donwload time of the data.

But definitely we should have tests checking the viewer components updates. @igoroctaviano @swederik are these type of tests already in place?

[igoroctaviano]

@Punzo you could use tools like profiler to keep track of how long that function took to execute and take notes. I think component rendering should be the last resort considering that the majority of studies load fine.

@JamesAPetts used to work on the VTK stuff in OHIF, if we are lucky he may have some insights on this issue.

[Punzo]

sure, I can measure the perfomances. But again my point is that calling N multiple times (where N is actually pretty large) the function _isDisplaySetReconstructable, called by VTKMPRToolbarButton, everytime we scroll in the cornerstone view an image does not make any sense (still have to check if it happens with other interactions). I would say that isDisplaySetReconstructable should be called only once, when we load the study. Independently by the heuristic and perfomance of _isDisplaySetReconstructable, the function should not be called more than one time. Maybe we can store its value in a temp variable for a given study.

[Punzo]

I analized the perfomances and in the case of 512x512x3000 voxels _isDisplaySetReconstructable takes ~7 sec to run for me. It is long, but at the moment we don't have better heuristic and we have to make a sequantial double loop for. I have also checked that the total time spent is mostly in _isDisplaySetReconstructable; so definitevely the slowness issue is there.

A better heuristic would be checking 4D tags, e.g. the presence of multiple TemporalPositionIdentifier values. However, some studies (e.g. https://github.com/OHIF/Viewers/issues/2113) do not have such tags. So, assuming that slices at different time have the same position, at the moment we just check if there are multiple slices for the same ImagePositionPatient and disable MPR.

I still believe that we should call _isDisplaySetReconstructable only once when we load the study/series and not N times everytime we do an interaction with the view.

[Punzo]

Just noticed/realized that there is already one PR https://github.com/OHIF/Viewers/pull/2274 , which should solve this (exactly calculating _isDisplaySetReconstructable only once and save it as a flag). @racacere @swederik what is the status of the PR?

[racacere]

@Punzo Hi, I submitted that PR a couples of weeks ago, but it haven't be reviewed yet. However, at my workplace we have been using this solution on our public OHIF fork for a while and it works quite well.

[fedorov]

A better heuristic would be checking 4D tags, e.g. the presence of multiple TemporalPositionIdentifier values. However, some studies (e.g. #2113) do not have such tags.

I am afraid there may be no robust alternative in the existing data to checking for overlapping frames.

But we could precompute "is 4D" flag for each series on IDC. It would not be in DICOM. Should we even consider this as an approach?

[pieper]

But we could precompute "is 4D" flag for each series on IDC. It would not be in DICOM. Should we even consider this as an approach?

Fixing it in OHIF makes much more sense to me.

[fedorov]

Sorry for the confusion, I meant doing that in addition to fixing repeated 4D checking in OHIF.

[Punzo]

@Punzo Hi, I submitted that PR a couples of weeks ago, but it haven't be reviewed yet. However, at my workplace we have been using this solution on our public OHIF fork for a while and it works quite well.

perfect, I will test and review. @igoroctaviano could you also review the PR, please?

Fixing it in OHIF makes much more sense to me.

I agree.

Sorry for the confusion, I meant doing that in addition to fixing repeated 4D checking in OHIF.

for sure we can add a 4d tags in IDC data, but I remind a couple of conversations that we didn't want to add custom IDC specific metadata outside from the DICOM standard in the data (we had this conversaion for the overlapping segmentation).

I would say once we merge the @racacere PR, the viewer should be responsive again and we can properly test again and see if we need to boost the perfomance with precomputed tags or not.

[fedorov]

Looks much better in 4.9.7! Instructions for performance regression testing have been added to IDC Viewer test docs, so closing this issue.