Since this is quite an invasive fix, I'd like Michiel's feedback on this before it gets merged.

@mdehoon, in case you were not aware of this PR, please take a look when you can.

I don't see anything obviously wrong with this pull request, so assuming it passed sufficient testing I would suggest to accept it.

@mdehoon Thanks for taking the time to look at the code! I really appreciate your thoughts.

It would be nice to fix this long-standing issue with the OS X backend. Perhaps this could make it into 1.2.x? Or, for want of not not being too hasty, milestoning it for 1.2.x and letting it garner more input I think would also be beneficial.

I agree that it would be nice to get it into 1.2 and be done with it. I don't have time for any testing now. I assume @dmcdougall has tested for basic functionality with all file types. If so, then the only other important test I can think of would be to run it on each file type in a long loop to check for memory leakage. Ideally this would be done on a range of OSX versions and architectures, but I think that waiting for "someone" to do this would be counterproductive. I would be satisfied with a single test for memory leakage with 1000 or so iterations on a reasonably up-to-date system. @dmcdougall, can you give that a try? We have cbook.report_memory for exactly this purpose.

@efiring Sure thing. I actually had only tested TIFF and BMP. I will check all files in a loop, as you suggest, and I'll report back here with stats when I'm done.

Watch this space.

Ok, here goes. Here's the script I wrote to check each bitmap type:

#!/opt/local/bin/python

import matplotlib
matplotlib.use('macosx')
import matplotlib.cbook as cbook
import matplotlib.pyplot as plt

cbook.report_memory()

fig = plt.figure()
ax = fig.add_subplot(1, 1, 1)
ax.plot([1, 2, 3])

filetypes = ['tiff', 'bmp', 'jpg', 'gif', 'png']

i = 0
for f in filetypes:
    while i < 1000:
        if i == 1:
            print 'Begin: Type', f, 'Mem usage', cbook.report_memory()
        if i == 999:
            print 'End: Type', f, 'Mem usage', cbook.report_memory()
        fig.savefig('fig.'+f)
        i += 1
    i = 0

Here's the stats before the change:

$ ./bitmap_memtest.py # before
Begin: Type tiff Mem usage 37808
End: Type tiff Mem usage 39156
Begin: Type bmp Mem usage 38212
End: Type bmp Mem usage 39332
Begin: Type jpg Mem usage 39336
End: Type jpg Mem usage 39336
Begin: Type gif Mem usage 40408
End: Type gif Mem usage 40432
Begin: Type png Mem usage 42972
End: Type png Mem usage 45112

Here's the change after:

$ ./bitmap_memtest.py # after
Begin: Type tiff Mem usage 38908
End: Type tiff Mem usage 39376
Begin: Type bmp Mem usage 39376
End: Type bmp Mem usage 39376
Begin: Type jpg Mem usage 37496
End: Type jpg Mem usage 37512
Begin: Type gif Mem usage 41388
End: Type gif Mem usage 41856
Begin: Type png Mem usage 42332
End: Type png Mem usage 44580

Though it increases steadily, it increases the same in each case. I conclude that I introduced no (new) memory leaks.

I also did a quick savefig('asd.pdf', dpi=1000) for each of the file types, and they all look fine. The metadata is wrong but the figure size is correct. The metadata shouldn't hold this up, and it's mentioned in another issue (of which I forgot the number).

Let's try to fix the memory leak. This is a good test case, and without fixing it we don't know if we added another memory leak by this bug fix.

Let's try to fix the memory leak. This is a good test case, and without fixing it we don't know if we added another memory leak by this bug fix.

I agree with the sentiment, but I would encourage whoever looks into this to do it as a separate package of work. (in most cases: many smaller PRs are better than one big one)

Yes, the memory leak needs to be fixed. I was deliberating whether to fix it in this pull request or not. I have some ideas of where to look. Since I already have the script, I'd be happy to look into this further.

Just a quick reminder, a couple of days ago, a user on the mailing list was
investigating an old memory leak of his, and found that it disappeared when
he upgraded to numpy v1.6.1. Perhaps a quick-n-dirty check would be to see
if the memory leak is numpy-version dependent?

Ok, well the results above were for numpy version 1.6.2. Here's some more:

$ ./bitmap_memtest.py # numpy 1.6.1
Begin: Type tiff Mem usage 39056
End: Type tiff Mem usage 39416
Begin: Type bmp Mem usage 39416
End: Type bmp Mem usage 39552
Begin: Type jpg Mem usage 37676
End: Type jpg Mem usage 37672
Begin: Type gif Mem usage 41408
End: Type gif Mem usage 41880
Begin: Type png Mem usage 42452
End: Type png Mem usage 44616

$ ./bitmap_memtest.py # numpy current master
Begin: Type tiff Mem usage 39372
End: Type tiff Mem usage 39648
Begin: Type bmp Mem usage 39648
End: Type bmp Mem usage 39652
Begin: Type jpg Mem usage 37772
End: Type jpg Mem usage 37772
Begin: Type gif Mem usage 41648
End: Type gif Mem usage 42188
Begin: Type png Mem usage 42692
End: Type png Mem usage 44864

So, I found some memory leaks, but not in the OS X backend:

Process:         Python [64109]
Path:            /opt/local/Library/Frameworks/Python.framework/Versions/2.7/Resources/Python.app/Contents/MacOS/Python
Load Address:    0x10a71a000
Identifier:      org.python.python
Version:         2.7.3 (2.7.3)
Code Type:       X86-64 (Native)
Parent Process:  bash [63564]

Date/Time:       2012-09-18 00:30:59.374 +0100
OS Version:      Mac OS X 10.7.4 (11E53)
Report Version:  7

leaks Report Version:  2.0
Process 64109: 30071 nodes malloced for 26611 KB
Process 64109: 6 leaks for 192 total leaked bytes.
Leak: 0x10aaa66b0  size=32  zone: DefaultMallocZone_0x10a922000   Py::ExtensionModuleBasePtr  C++  _path.so
    0x0b9e2ea0 0x00000001 0x00000001 0x00000000     ................
    0x314671d0 0x00007fdb 0x314572e0 0x00007fdb     .qF1.....rE1....
Leak: 0x7fdb314349d0  size=32  zone: DefaultMallocZone_0x10a922000   Py::ExtensionModuleBasePtr  C++  _path.so
    0x0b9e2ea0 0x00000001 0x00000001 0x00000000     ................
    0x314671d0 0x00007fdb 0x31465590 0x00007fdb     .qF1.....UF1....
Leak: 0x7fdb3147a820  size=32  zone: DefaultMallocZone_0x10a922000   Py::ExtensionModuleBasePtr  C++  _path.so
    0x0b9e2ea0 0x00000001 0x00000001 0x00000000     ................
    0x314671d0 0x00007fdb 0x31479c30 0x00007fdb     .qF1....0.G1....
Leak: 0x7fdb314f6be0  size=32  zone: DefaultMallocZone_0x10a922000   Py::ExtensionModuleBasePtr  C++  _path.so
    0x0b9e2ea0 0x00000001 0x00000001 0x00000000     ................
    0x314671d0 0x00007fdb 0x314d34e0 0x00007fdb     .qF1.....4M1....
Leak: 0x7fdb316e6fa0  size=32  zone: DefaultMallocZone_0x10a922000   Py::ExtensionModuleBasePtr  C++  _path.so
    0x0b9e2ea0 0x00000001 0x00000001 0x00000000     ................
    0x314671d0 0x00007fdb 0x316db400 0x00007fdb     .qF1......m1....
Leak: 0x7fdb31789e00  size=32  zone: DefaultMallocZone_0x10a922000   Py::ExtensionModuleBasePtr  C++  _path.so
    0x0b9e2ea0 0x00000001 0x00000001 0x00000000     ................
    0x314671d0 0x00007fdb 0x317b36b0 0x00007fdb     .qF1.....6{1....

They appear to be coming from the _path.so file, from the Py::ExtensionModuleBasePtr function. In that function some memory is allocated in line 224:

PyObject *module_ptr = new ExtensionModuleBasePtr( this );

I have no idea where this pointer is freed. Does anybody more experienced have any idea?

To keep things separate, I strongly suggest that memory leak identification gets its own issue.

@dmcdougall : Is this code in this PR complete? If so, I will merge. Alternatively, if it depends on another issue, such as the memory leak one, then we can hold this PR until that one is complete.

Cheers,

I even see similar leaks in the Agg backend, though admittedly, not as bad. I think the source of the memory leak is deeper.

I think this code is complete and this PR does not depend on another issue. The memory leak doesn't seem to be constrained to the mac backend.

Merged to master. @efiring mentioned he may like to see this on 1.2.x, which would obviously need another piece of work (to open the PR). It may be worth clarifying here first if that is still the case.

We don't need another PR to put it on 1.2.x -- it can just be merged there. I think I agree with @efiring that it should be. I'll go ahead and do that.