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

Fix a number of memory leaks. #4862

Merged
merged 7 commits into from Feb 1, 2017

Conversation

Projects
None yet
7 participants
@jdf
Contributor

jdf commented Jan 29, 2017

These leaks don't matter--could never be noticed--in Java Mode, which just System.exit()s when the sketch is done. But the Python Mode sketch runner sticks around between invocations, which makes it susceptible to severe memory loss when, for example, many PShapes are created.

This patch does a few things.

Most noticeably, it refactors PGraphicsOpenGL's resource disposal technique to remove a ton of duplicated code. But it also throws away the "RefList" which not only didn't do a desirable thing, but kept resources from ever being released (since there was no way for resources to be removed from the RefList other than by their being disposed() which could only happen if they were released from the RefList!).

It also terminates an exception-catching thread at shutdown time (which otherwise keeps running forever, and never lets go of its closure on PGraphicsOpenGL).

It clears a closure on PApplet from the ThinkDifferent QuitHandler.

This patch fixes jdf/processing.py#233.

@benfry

This comment has been minimized.

Show comment
Hide comment
@benfry

benfry Jan 30, 2017

Member

Awesome; this is great work, thank you.

@codeanticode Can you review the parts of the OpenGL parts? If they look good, we're good to merge.

Member

benfry commented Jan 30, 2017

Awesome; this is great work, thank you.

@codeanticode Can you review the parts of the OpenGL parts? If they look good, we're good to merge.

@codeanticode

This comment has been minimized.

Show comment
Hide comment
@codeanticode

codeanticode Jan 30, 2017

Member

yep, will review asap! Thanks for these fixes, @jdf

Member

codeanticode commented Jan 30, 2017

yep, will review asap! Thanks for these fixes, @jdf

@jdf

This comment has been minimized.

Show comment
Hide comment
@jdf

jdf Jan 30, 2017

Contributor

Don't thank me until it's been proven not to crash. :)

Contributor

jdf commented Jan 30, 2017

Don't thank me until it's been proven not to crash. :)

@codeanticode

This comment has been minimized.

Show comment
Hide comment
@codeanticode

codeanticode Jan 30, 2017

Member

@jdf The refactoring to reduce code duplication looks good to me. However, the use of the RefList is simply following the technique described in the original article:

"Notice that reference objects are discovered by the garbage collector and added to their associated reference queues only if they are reachable themselves. Otherwise, they are simply reclaimed like any other unreachable object. This is why you add all NativeImage3 instances to the static list -- actually, any data structure will suffice -- to ensure that they remain reachable and processed when their referents become unreachable. Naturally, you also have to make sure that you remove them from the list when you dispose of them. This is done in the dispose() method."

Member

codeanticode commented Jan 30, 2017

@jdf The refactoring to reduce code duplication looks good to me. However, the use of the RefList is simply following the technique described in the original article:

"Notice that reference objects are discovered by the garbage collector and added to their associated reference queues only if they are reachable themselves. Otherwise, they are simply reclaimed like any other unreachable object. This is why you add all NativeImage3 instances to the static list -- actually, any data structure will suffice -- to ensure that they remain reachable and processed when their referents become unreachable. Naturally, you also have to make sure that you remove them from the list when you dispose of them. This is done in the dispose() method."

@jdf

This comment has been minimized.

Show comment
Hide comment
@jdf

jdf Jan 30, 2017

Contributor

Understood. Will ping again when I've corrected it.

Contributor

jdf commented Jan 30, 2017

Understood. Will ping again when I've corrected it.

@jdf

This comment has been minimized.

Show comment
Hide comment
@jdf

jdf Jan 30, 2017

Contributor

OK; please take another look.

Contributor

jdf commented Jan 30, 2017

OK; please take another look.

@JakubValtar

This comment has been minimized.

Show comment
Hide comment
@JakubValtar

JakubValtar Jan 30, 2017

Contributor

I think it's fixed. However, you also need to check type of argument in equals(), since you are mixing all the subclasses in one list, otherwise you get ClassCastException in reachableWeakReferences.remove(this);

Contributor

JakubValtar commented Jan 30, 2017

I think it's fixed. However, you also need to check type of argument in equals(), since you are mixing all the subclasses in one list, otherwise you get ClassCastException in reachableWeakReferences.remove(this);

@JakubValtar

This comment has been minimized.

Show comment
Hide comment
@JakubValtar

JakubValtar Jan 30, 2017

Contributor

If in doubt, run this stress test with -Xmx32m and print debug message in dispose()

  public void settings() {
    size(640, 480, P3D);
    smooth(2);
  }

  public void setup() {
  }

  public void draw() {
    PGraphicsOpenGL pg = (PGraphicsOpenGL)createGraphics(width, height, P3D);
    pg.beginDraw();
    pg.background(random(255), random(255), random(255));
    pg.endDraw();
    image(pg, 0, 0);
  }

(If your code is wrong you need to stop it after ~10 seconds, otherwise it might crash your computer as it crashed mine.)

Contributor

JakubValtar commented Jan 30, 2017

If in doubt, run this stress test with -Xmx32m and print debug message in dispose()

  public void settings() {
    size(640, 480, P3D);
    smooth(2);
  }

  public void setup() {
  }

  public void draw() {
    PGraphicsOpenGL pg = (PGraphicsOpenGL)createGraphics(width, height, P3D);
    pg.beginDraw();
    pg.background(random(255), random(255), random(255));
    pg.endDraw();
    image(pg, 0, 0);
  }

(If your code is wrong you need to stop it after ~10 seconds, otherwise it might crash your computer as it crashed mine.)

@JakubValtar

This comment has been minimized.

Show comment
Hide comment
@JakubValtar

JakubValtar Jan 30, 2017

Contributor

Looks good, thanks @jdf. Let's wait what @codeanticode thinks.

However, it does the same thing as before (just with less code), so you have to check if it fixes your problem. The code worked well previously (we tested it with @codeanticode before 3.0 came out), so I'm not sure how the leak could occur. Everything should be released in next garbage collector run, unless there is some other sneaky problem. Edit: the exception thread was probably the reason.

Contributor

JakubValtar commented Jan 30, 2017

Looks good, thanks @jdf. Let's wait what @codeanticode thinks.

However, it does the same thing as before (just with less code), so you have to check if it fixes your problem. The code worked well previously (we tested it with @codeanticode before 3.0 came out), so I'm not sure how the leak could occur. Everything should be released in next garbage collector run, unless there is some other sneaky problem. Edit: the exception thread was probably the reason.

@jdf

This comment has been minimized.

Show comment
Hide comment
@jdf

jdf Jan 30, 2017

Contributor

@JakubValtar First, thanks very much for your help.

The test you suggested worked well to verify disposals taking place. (Although for Python Mode I had to give it 48M to run at all :} ).

I think the real problem was the non-exiting thread that waited forever for an unhandled exception to be thrown (while closed over its containing PGraphicsOpenGL), along with the Apple-specific one.

Contributor

jdf commented Jan 30, 2017

@JakubValtar First, thanks very much for your help.

The test you suggested worked well to verify disposals taking place. (Although for Python Mode I had to give it 48M to run at all :} ).

I think the real problem was the non-exiting thread that waited forever for an unhandled exception to be thrown (while closed over its containing PGraphicsOpenGL), along with the Apple-specific one.

@JakubValtar

This comment has been minimized.

Show comment
Hide comment
@JakubValtar

JakubValtar Jan 30, 2017

Contributor

@jdt You're welcome :) I ran it through VisualVM (great for heap analysis) and I think you also need to clear the exception handler in PAppletJythonDriver at line 628.

Contributor

JakubValtar commented Jan 30, 2017

@jdt You're welcome :) I ran it through VisualVM (great for heap analysis) and I think you also need to clear the exception handler in PAppletJythonDriver at line 628.

@jdf

This comment has been minimized.

Show comment
Hide comment
@jdf

jdf Jan 30, 2017

Contributor

@JakubValtar I don't know who @jdt is, but I believe they spend a lot of time working in Eclipse. :)

Yes, I know about the PAppletJythonDriver fix; I've got that going in my own repo. Again, thank you.

For what it's worth, I use YourKit for memory debugging (and other profiling tasks). I'll also download VisualVM and give it a try.

Contributor

jdf commented Jan 30, 2017

@JakubValtar I don't know who @jdt is, but I believe they spend a lot of time working in Eclipse. :)

Yes, I know about the PAppletJythonDriver fix; I've got that going in my own repo. Again, thank you.

For what it's worth, I use YourKit for memory debugging (and other profiling tasks). I'll also download VisualVM and give it a try.

@JakubValtar

This comment has been minimized.

Show comment
Hide comment
@JakubValtar

JakubValtar Jan 30, 2017

Contributor

Oops, sorry :)

VisualVM is part of JDK, at least on Windows (jvisualvm.exe). I never used YourKit but I think it's better than whatever ships with JDK.

Contributor

JakubValtar commented Jan 30, 2017

Oops, sorry :)

VisualVM is part of JDK, at least on Windows (jvisualvm.exe). I never used YourKit but I think it's better than whatever ships with JDK.

@jdt

This comment has been minimized.

Show comment
Hide comment
@jdt

jdt Jan 31, 2017

I have no idea what this project is, but I wholeheartedly support the elimination of memory leaks.

jdt commented Jan 31, 2017

I have no idea what this project is, but I wholeheartedly support the elimination of memory leaks.

@codeanticode

This comment has been minimized.

Show comment
Hide comment
@codeanticode

codeanticode Jan 31, 2017

Member

Yes, I think the problem was the exception-catching thread, seems to me that now is properly terminated when the animation thread stops.

@benfry I think the GL fixes are ready to be merged.

Member

codeanticode commented Jan 31, 2017

Yes, I think the problem was the exception-catching thread, seems to me that now is properly terminated when the animation thread stops.

@benfry I think the GL fixes are ready to be merged.

@benfry benfry merged commit deef9de into processing:master Feb 1, 2017

@benfry

This comment has been minimized.

Show comment
Hide comment
@benfry

benfry Feb 1, 2017

Member

Great; merged.

Member

benfry commented Feb 1, 2017

Great; merged.

@codeanticode

This comment has been minimized.

Show comment
Hide comment
@codeanticode

codeanticode Feb 1, 2017

Member

Just noticed that the System.err.println() line is still present in Disposable.dispose() https://github.com/processing/processing/blob/master/core/src/processing/opengl/PGraphicsOpenGL.java#L82

@jdf Shouldn't be removed?

Member

codeanticode commented Feb 1, 2017

Just noticed that the System.err.println() line is still present in Disposable.dispose() https://github.com/processing/processing/blob/master/core/src/processing/opengl/PGraphicsOpenGL.java#L82

@jdf Shouldn't be removed?

@jdf

This comment has been minimized.

Show comment
Hide comment
@jdf

jdf Feb 1, 2017

Contributor

Yes.

Contributor

jdf commented Feb 1, 2017

Yes.

@neilcsmith-net

This comment has been minimized.

Show comment
Hide comment
@neilcsmith-net

neilcsmith-net Aug 8, 2017

@jdf I've just been looking at this pull request and testing the latest version of the Python mode, and there is definitely a memory leak still there! Praxis LIVE actually does a similar thing to the Python mode here, and I need to update my fix hence looking at this. I'm using reflection to clean out the reference lists and remove the PGL references in the resource classes at the end of each sketch run.

One problem is that the reference list is a static field - when I suggested using this method of cleanup to @codeanticode I suggested using instance fields, perhaps on PSurfaceJOGL, and explicitly disposing. The second issue is that the resource classes have a strong reference to PGL in them which creates a circular reference that stops a lot of things being cleaned up. Of course, if the second issue didn't exist, you'd end up cleaning up resources that no longer exist because the JOGL context has gone, so in some ways it's a memory leak masking something worse.

neilcsmith-net commented Aug 8, 2017

@jdf I've just been looking at this pull request and testing the latest version of the Python mode, and there is definitely a memory leak still there! Praxis LIVE actually does a similar thing to the Python mode here, and I need to update my fix hence looking at this. I'm using reflection to clean out the reference lists and remove the PGL references in the resource classes at the end of each sketch run.

One problem is that the reference list is a static field - when I suggested using this method of cleanup to @codeanticode I suggested using instance fields, perhaps on PSurfaceJOGL, and explicitly disposing. The second issue is that the resource classes have a strong reference to PGL in them which creates a circular reference that stops a lot of things being cleaned up. Of course, if the second issue didn't exist, you'd end up cleaning up resources that no longer exist because the JOGL context has gone, so in some ways it's a memory leak masking something worse.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment