search

openmc-dev / plotter

Native plotting GUI for model design and verification
MIT License
46 stars 18 forks source link

Improve performance for large models with DomainViewDict #138

Closed paulromano closed 6 months ago

paulromano commented 6 months ago

If you run the plotter on a model with lots of cells (the ITER E-lite model, for example), it spends a lot of time copying data around unnecessarily. The crux of the issue is this line: https://github.com/openmc-dev/plotter/blob/e772fb9ba55532ea984c4eb529c7fbf2982268f0/openmc_plotter/plotmodel.py#L321

which deepcopies the entire PlotView object. This object holds dictionaries called cells and materials that map cell and material IDs to DomainView objects (one for each cell and material that appears in the model). When I plot the ITER E-lite model, which has >300k cells, those deepcopies take about 12 seconds on my machine, which means that every time I update the plot view, there is a lag of 12 seconds even when the plot resolution is super low.

This PR makes a fairly easy fix to this problem, which is to store a global dictionary that holds the default DomainView view settings that are created at startup separately from the PlotView object itself. If you make a modification to the domain view (e.g., changing the color of a material), it does get stored as part the PlotView object and will get copied around, but the default domain views remain untouched and don't get copied.

paulromano commented 6 months ago

Here's what it looks like working with E-lite on my laptop: plotter_domainviewdict

paulromano commented 6 months ago

Just added a commit which shaves off about ~7 sec of initialization time for the E-lite model

paulromano commented 6 months ago

...and just made another commit that shaved another 7 sec off initialization. Generating the default DomainView objects for each domain took about 20 sec before for the E-lite model and now it's down to 5 sec.

pshriwise commented 6 months ago

Good idea to de-couple those from the PlotView's -- no reason we need to bring them along in the copy. Any reason we shouldn't put those global dictionaries on the PlotModel object?

paulromano commented 6 months ago

@pshriwise The global dictionaries are accessed from the DomainViewDict class, which is separate from the PlotModel class

pshriwise commented 6 months ago

@pshriwise The global dictionaries are accessed from the DomainViewDict class, which is separate from the PlotModel class.

Understood, but any reason not to make the DomainViewDict class an attribute of the PlotModel class? It seems it would be more in line with the MVC approach this app is intending to take.

paulromano commented 6 months ago

Ok, I was able to come up with a better (I think?) solution here. Instead of storing the defaults as a global dictionary, they are stored as an attribute on DomainViewDict. To avoid the original issue with all the defaults being deepcopied, I overrode the __deepcopy__ method to make it only copy a reference of the defaults dictionary. Let me know what you think now @pshriwise