Refactor volume rendering to eliminate need for ImageContainer struct

[ngoldbaum]

Closes #1374

To evaluate this PR consider the following script:

import yt
from yt.testing import fake_random_ds

from pympler import tracker

tracker = tracker.SummaryTracker()

ds = fake_random_ds(16)
sc = yt.create_scene(ds)
sc.render()
del sc
del ds

tracker.print_diff()

This is a little bit different from the script in #1374 in that it has the nice feature that the stuff between the creation of the SummaryTracker and the call to tracker.print_diff should in principle have no side effects.

Before this PR I get the following output:

                                                   types |   # objects |   total size
========================================================= | =========== | ============
                                    <class 'numpy.ndarray |          10 |      6.01 MB
                                             <class 'list |       12003 |      1.11 MB
                                              <class 'str |       12509 |    904.12 KB
                                             <class 'dict |         151 |    117.38 KB
                                              <class 'int |        3028 |     84.93 KB
                                            <class 'tuple |         895 |     73.38 KB
                 <class 'sympy.core.assumptions.StdFactKB |         114 |     59.72 KB
                         <class 'functools._lru_list_elem |         614 |     47.97 KB
                        <class 'yt.units.yt_array.YTArray |           9 |     33.15 KB
                        <class 'yt.units.unit_object.Unit |         133 |     15.59 KB
                                             <class 'type |           0 |     12.00 KB
                               <class 'sympy.core.mul.Mul |         126 |      8.86 KB
                             <class 'sympy.core.power.Pow |          89 |      6.26 KB
                                            <class 'float |         127 |      2.98 KB
  <class 'yt.data_objects.static_output.RegisteredDataset |           0 |      2.91 KB

And after, I get:

                                                    types |   # objects |   total size
========================================================= | =========== | ============
                                             <class 'list |       11996 |      1.11 MB
                                              <class 'str |       12509 |    904.12 KB
                                             <class 'dict |         140 |    115.78 KB
                                              <class 'int |        3004 |     84.18 KB
                                            <class 'tuple |         857 |     70.23 KB
                 <class 'sympy.core.assumptions.StdFactKB |         108 |     53.44 KB
                         <class 'functools._lru_list_elem |         588 |     45.94 KB
                        <class 'yt.units.yt_array.YTArray |           7 |     32.88 KB
                        <class 'yt.units.unit_object.Unit |         133 |     15.59 KB
                                    <class 'numpy.ndarray |           5 |     12.62 KB
                                             <class 'type |           0 |     12.00 KB
                               <class 'sympy.core.mul.Mul |         123 |      8.65 KB
                             <class 'sympy.core.power.Pow |          89 |      6.26 KB
                                            <class 'float |         127 |      2.98 KB
  <class 'yt.data_objects.static_output.RegisteredDataset |           0 |      2.91 KB

The major difference here is that we're no longer leaking 6 MB of ndarray data. The rest of the "leaked" memory are I suspect in global caches in the yt module or in sympy.

[ngoldbaum]

Ping @samskillman. I realize that you're super busy, but this is a pretty big refactoring of the volume renderer's internals and I'd appreciate your eyes on this.

The problem with ImageContainer is that it's a C struct with members that are python objects (the memoryviews). I think Cython isn't appropriately decrementing the reference counts to these objects when the ImageContainer is ultimately freed by the VR machinery.

Rather than trying to work around cython issues, I think it makes more sense to simply move all the members of the ImageContainer class to the ImageSampler class.

[matthewturk]

The two reasons I remember for actually using a struct were for timing (to avoid lots of property lookups in loops) and for openmp parallelism. Can you check that the timings are still okay with this?

[ngoldbaum]

Here's a crude benchmark:

import yt

ds = yt.load('Enzo_64/DD0043/data0043')

yt.volume_render(ds)

Before this PR:

3088.11s user 32.88s system 2490% cpu 2:05.32 total

After this PR:

3102.61s user 28.71s system 2534% cpu 2:03.55 total

So at least for this test I don't see any appreciable difference.

[samskillman]

I think this looks ok to me, though I have to be honest there are a lot of cobwebs that I'm not breaking through. Just a quick question -- if you did im = sc.render(); del im; in the script above would it make a difference? Probably not?

[ngoldbaum]

Hey @samskillman thanks so much for taking a look!

Here's what I think you were suggesting:

import yt
from yt.testing import fake_random_ds

from pympler import tracker

tracker = tracker.SummaryTracker()

ds = fake_random_ds(16)
sc = yt.create_scene(ds)
im = sc.render()
del sc
del ds
del im

tracker.print_diff()

And here the output before this PR: http://paste.yt-project.org/show/7177/

And after: http://paste.yt-project.org/show/7178/

tl;dr: there's still a memory leak even if we explicitly get an image back from the rendering machinery.