Optimizing performance of events when clearing nodes

I often have about 100 hundred volumes loaded into the scene with corresponding segmentations for each of those volumes. When clearing nodes I’ve observed varying performance based on how many times the current selection in a qMRMLNodeComboBox keeps updating. This is because when a qMRMLNodeComboBox’s current selection is removed from the scene, it selects the last available index in the model and processes that update change to the widgets involved. This additional processing can add up if I happen to be clearing nodes where the current node selection in the combobox is the one being removed.

A specific example is the volume node comboboxes part of slice viewers. If I I’m removing a set of volumes nodes in the scene where the order corresponds to the current selection, then it can result in a flashing effect as the current selection is removed, the last index is selected, the new volume is shown in the slice viewer, then the new selection is removed, the last index is selected again, a new volume is shown in the slice viewer, etc… This additional processing happens as well for all other node comboboxes such as the Master Volume node combobox in the Segment Editor module.

Has there been discussions in the past about how best to handle unnecessary updates like this? I know it is standard behavior for a QComboBox to select the last index when the current selection is removed, but this increases processing if there are events tied to the index being changed. If a volume node is selected in various qMRMLNodeComboBoxes across various modules, then when that volume is removed it will trigger updates across all of those modules that I might not care to process. Maybe I want a “None” type selection when the current selection is removed.

Things that I have played around with is when batch removing a set of volumes in the scene I call blockSignals(True) on the qMRMLNodeComboBoxes in the slice viewers and in the SegmentEditor module and then at the end call blockSignals(False) to reset the state and then call a single update for the selection I want at the end of the remove node process.

This is a targeted approach, but doesn’t find all the places that might be processing updates with new volume selections during the batch removal of volume nodes.

I’m calling the individual slicer.mrmlScene.RemoveNode(volumeNode), but maybe there could be a RemoveNodes method available to use that would process the list, but only trigger a single update for each of the relevant qMRMLNodeComboBoxes.

Could use your approach finding all mrmlNodeComboBoxes with mainWindow().findChildren(…) ?

Do you switch the scene into batch processing state when you remove the nodes?

scene->StartState(vtkMRMLScene::BatchProcessState);
...
(many changes in the scene)
...
scene->EndState(vtkMRMLScene::BatchProcessState);
2 Likes

I did not know about the vtkMRMLScene::BatchProcessState so I have not been using that. I just tried putting it around my batch clearing of volume and segmentation nodes method and can confirm I see it working by not processing slice view updates, but my profiling is still indicating multiple calls of masterVolumeNodeChanged coming from Segment Editor stuff that is cumulatively about 25% of the time to clear.

What does this BatchProcessState specifically prevent? Just visual changes in the slice views? Or other mrml objects such as qMRMLNodeComboBoxes in various modules from triggering multiple times during batch processing.

@jamesobutler definitely try the batch process mode, it was designed exactly for the use case you describe. Not just comboboxes but other updates like views are deferred until after the bulk operation is done.

Every module, logic, widget, etc. can decide how to utilize scene state information. The idea is that while the scene is in batch processing state, software components can ignore scene changes and do a full update when batch processing is finished.

It seems that the Segment Editor could do some more optimization based on the scene state. There are a number of places where such optimizations are done (see for example qSlicerMarkupsModuleWidget) that you can use as examples.

Thanks for pointing this out. I will try some changes to add the check for batch processing in some more places and will issue a PR if I’m successful.

1 Like

@lassoan Is there a method for making sure the widgets get updated at the end of batch processing? When trying it out with the Markups module (qSlicerMarkupsModuleWidget) as the currently selected module I’m observing that the qMRMLSubjectHierarchyTreeView isn’t updated when the batch processing state is ended. Is this normal behavior or a bug? I would have expected that when BatchProcessingState is ended that the markups table would show the node for “First” and “Fourth”.

first=slicer.mrmlScene.AddNewNodeByClass("vtkMRMLMarkupsFiducialNode", "First")
second=slicer.mrmlScene.AddNewNodeByClass("vtkMRMLMarkupsFiducialNode", "Second")
third=slicer.mrmlScene.AddNewNodeByClass("vtkMRMLMarkupsFiducialNode", "Third")
slicer.mrmlScene.StartState(slicer.mrmlScene.BatchProcessState)

for node in [third, second]:
  slicer.mrmlScene.RemoveNode(node)

slicer.mrmlScene.EndState(slicer.mrmlScene.BatchProcessState)
fourth=slicer.mrmlScene.AddNewNodeByClass("vtkMRMLMarkupsFiducialNode", "Fourth")

image

The code above where the qMRMLSubjectHierarchyTreeView isn’t updated upon ending the BatchProcessingState produces the following errors.

[DEBUG][Qt] 25.05.2022 18:24:45 [] (unknown:0) - Python console user input: slicer.mrmlScene.EndState(slicer.mrmlScene.BatchProcessState)
[ERROR][VTK] 25.05.2022 18:24:45 [vtkMRMLSubjectHierarchyNode (0000028BAFF77F30)] (D:\D\P\S-0\Libs\MRML\Core\vtkMRMLSubjectHierarchyNode.cxx:2114) - GetItemDataNode: Failed to find subject hierarchy item by ID 8
[ERROR][VTK] 25.05.2022 18:24:45 [vtkMRMLSubjectHierarchyNode (0000028BAFF77F30)] (D:\D\P\S-0\Libs\MRML\Core\vtkMRMLSubjectHierarchyNode.cxx:2827) - GetItemParent: Failed to find subject hierarchy item by ID 8
[ERROR][VTK] 25.05.2022 18:24:45 [vtkMRMLSubjectHierarchyNode (0000028BAFF77F30)] (D:\D\P\S-0\Libs\MRML\Core\vtkMRMLSubjectHierarchyNode.cxx:2114) - GetItemDataNode: Failed to find subject hierarchy item by ID 9
[ERROR][VTK] 25.05.2022 18:24:45 [vtkMRMLSubjectHierarchyNode (0000028BAFF77F30)] (D:\D\P\S-0\Libs\MRML\Core\vtkMRMLSubjectHierarchyNode.cxx:2827) - GetItemParent: Failed to find subject hierarchy item by ID 9

Looks like I’m possibly running into known issues as written up in https://github.com/Slicer/Slicer/issues/6006 and https://github.com/Slicer/Slicer/issues/4650.

Using the workaround mentioned in https://github.com/Slicer/Slicer/issues/6006, I can get the subject hierarchy in the Markups module to refresh.

Force subject hierarchy tree rebuild by temporarily switching to scene import mode:

slicer.mrmlScene.StartState(slicer.vtkMRMLScene.ImportState)
slicer.mrmlScene.EndState(slicer.vtkMRMLScene.ImportState)

image

All the classes that may ignore updates in batch processing mode must observe the scene and do a full update when batch processing is finished.

Unfortunately, the Subject Hierarchy scene model and widget were not developed with having batch processing mode in mind. It was a huge hole in the design and implementation. The code was later fixed up, so most of the major issues are fixed, but since it was just an afterthought and the implementation is quite complex, there may be still a few errors.

Is setting the BatchProcessState encouraged to be used as global state where you turn it on, go do a bunch of things, and then turn it off (see functionThatDoesAllThings)? Or should it be turned on/off only around the individual action (see function1)? The latter requires more sprinkling of the Start/End state methods.

In the case below when running functionThatDoesAllThings I would expect function2 actions to be skipped over, but it seems as though the batch process state would have been ended at the end of function1. In my own code I was setting the state like in functionThatDoesAllThings, but seems like in some SubjectHierarchy related code it does it like function1.

def function1():
    slicer.mrmlScene.StartState(slicer.mrmlScene.BatchProcessState)
    # do some things here
    slicer.mrmlScene.EndState(slicer.mrmlScene.BatchProcessState)

def function2():
    if slicer.mrmlScene.IsBatchProcessing():
        return
    # do some things here

def functionThatDoesAllThings():
    slicer.mrmlScene.StartState(slicer.mrmlScene.BatchProcessState)
    function1()
    function2()
    function1()
    slicer.mrmlScene.EndState(slicer.mrmlScene.BatchProcessState)

vtkMRMLScene::StartState is actually pushing on a stack, so the final EndState call should invoke the event as long as the starts and ends are paired.

Ah, yes I see that. Seems like I can use it around smaller methods or even in methods that do lots of subtasks.

def function1():
    slicer.mrmlScene.StartState(slicer.mrmlScene.BatchProcessState)
    print("Function 1 actions")
    slicer.mrmlScene.EndState(slicer.mrmlScene.BatchProcessState)

def function2():
    if slicer.mrmlScene.IsBatchProcessing():
        return
    print("Function 2 actions")
    # do some things here

def functionThatDoesAllThings():
    slicer.mrmlScene.StartState(slicer.mrmlScene.BatchProcessState)
    function1()
    function2()
    function1()
    slicer.mrmlScene.EndState(slicer.mrmlScene.BatchProcessState)
>>> functionThatDoesAllThings()
Function 1 actions
Function 1 actions

Reviewing the other mrml states:

It seems like some of my actions of batch processing nodes to be removed from the scene or added to the scene isn’t quite like CloseState and ImportState which appear to have been desired with clearing and importing a mrml scene. I’m doing some batch actions outside of mrml file use. It seems like in some places in the code there are checks for isClosing() and isImporting() which wouldn’t return true if Starting/Ending the BatchProcess state directly.

^ I’ve issued the following PR with an attempt to fix this update issue of the subject hierarchy view.

I’ve posted the following PR that addresses this issue of some unnecessary SegmentEditor signaling occurring during batch removing a bunch of volume nodes.

Thank you for all your work on this. I’ve already reviewed one of your pull request and will have a look at the others, too.

We haven’t had any use case in mind that would involve having hundreds of volumes and segmentations in the scene. Having a large number of nodes would cause performance and GUI usability issues at many other places, too. Can you tell a bit more about your application? Why do you have so many volumes?

Have you considered storing them in sequences (just pulling into the main scene those that you actually need) or merging these nodes into fewer nodes (e.g., reconstruct volumes from them, or put them into a single textured model node, or dynamically loading them into the scene when they are actually needed, …)?

Typically it represents volumes for a single timepoint of a cohort of mice. Where there is 25-50 mice in a cohort. Typically there is a 3D B-Mode ultrasound as a reference background for some other supporting mode (think like Color Flow, Shear Wave Elastography, M-Mode, etc).

You could load in a few volumes for a certain number of mice to keep the number of nodes down, but most of the researchers just load in all the data for that timepoint that they need to segment.

We have some custom stuff to improve workflows where we identify related volumes based on our metadata to appropriately pair volumes. So we don’t have a lot of combo boxes with 100s of selections to choose from. A primary volume with a subject hierarchy is used to organize nodes so we can show the relevant nodes when needed.