-
Notifications
You must be signed in to change notification settings - Fork 276
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
wrapping Image.copy causing an adverse impact on memory use #63
Comments
I've spent some time pondering this - it is tricky - exif data is specific only to some jpegs, but the processor pipeline expects to chain just a generic PIL image - which does not always have exif data. The above referenced commit tries to do this by wrapping copy to include the _getexif method on the copy, however:
I think the processor pipeline design is good, but it requires that there is a simple contract that a processor.process method takes a generic PIL image object and returns the same. exif data breaks that contract. there is no reliable way to pass this through the pipeline I think we need something akin to the autoconvert processor, or even build exif transpose into the autoconvert processor, it needs a first byte at the apple, and in that case ProcessPipleline can handle the image.copy() step. However it is not entirely clear to me why the pipeline needs to start with a copy of the image, instead of just the image itself? The other thing being done in the wrapping of the .copy() method is setting of an .app attr - but for the life of me, I can find no other reference to the use of this attribute in imagekit - is it just spurious? |
Indeed, the memory footprint is increased. My guess is that it's a side effect of wrapping a reference to the parent instance into the However, I don't think this constitutes a memory leak because, as is illustrated below, these reference cycles are cleaned up by the garbage collector. Here are some memory usage numbers under three different scenarios (a sample was taken on each iteration, then the samples were analyzed after the iteration completed): Scenario 1Using the
Scenario 2Using the wrapped version of
Scenario 3Using the wrapped version of
The increased memory usage in the second example above is likely a result of the garbage collector simply deferring collection until some later time. The numbers in that second example are markedly higher than those for the
And again, later in the cycle:
The memory levels rise to ~300M, then drop to ~150M repeatedly, which shows that the reference cyles are detected and cleaned up by the garbage collector. So it seems that the memory footprint is increased by the wrapped version of Here is the modified test case i used (you will need to install psutil to run it):
I ran scenario 1 by modifying I ran scenario 2 by using the I ran scenario 3 like scenario 2, but uncommented the line in the test case that forces garbage collection: |
You are correct that this is not a leak, and I've updated the issue accordingly. For background, the discovery of this came when trying to use imagekit to batch something on ep.io and the process runner/monitors they used were killing the task due to the high memory use. Thank you for performing the more detailed analysis. I still consider this adverse situation, and an issue. Using gc would be a workaround - but I still feel there are some non-memory issues with wrapping copy:
I support the idea of using the exif data, and I don't mind using PILs access to it (as compared to requiring another dependency, or reinventing) - but I think it should be as some sort of special pre-pipeline step, and can be done without the ~7x memory increase. |
Yeah, the copy-wrapping thing was definitely something that we weren't happy about. This was our attempt to work around a PIL limitation. Our attitude regarding the data not being preserved was basically just "auto transpositions have to be done first"—a restriction which should at the very least be pointed out in the documentation. I'll have to think about this. I can't really see a good way to avoid the kludgy pseudo-monkeypatching just yet. As far as the "app" attribute, I believe that was just another attribute that @lettertwo found PIL's |
Haha, @matthewwithanm beat me to this one by a few mins. Here are my thoughts anyway:
Well, that is the nature (and even the point) of the pipeline--to manipulate the data as it moves through. The problem we're addressing with this
Yes, it is gross. We found this to be the least bad of the approaches we considered when writing it. We'll all have to ponder this one a bit more.
The See Wikipedia's article on JPEG files for further explanation. While there is seperate access for metadata that conforms to the EXIF standard (the Since we are making an effort to preserve the EXIF metadata, it makes sense to also conserve the JFIF metadata. While none of the processors packaged with ImageKit (so far) make use of JFIF metadata, it's entirely possible that some processor in the future may want access to that metadata during processing, in which case, it should be made available. |
I lost a lot of time tracking down excessive memory usage on a server, eventually discovering that this ticket was the cause of it all. A simple |
The copy method is no longer wrapped in IK3, so I'm going to close this. |
f570bd0 changed the way images were opened, and wraps the original copy method with one that tries to preserve extra imagekit related info.
however this results in the image data being retained somehow in memory - I'm not sure exactly how or where.
commenting out this line:
img.copy = types.MethodType(_wrap_copy(img.copy), img, img.__class__)
results in a substantially reduced memory footprint
I've created a quick sample app/project to demonstrate this - if you watch the process memory - you can see it climb.
https://github.com/ptone/imagefoobar (note this is using the new Django 1.4/trunk layout)
cc @lettertwo
The text was updated successfully, but these errors were encountered: