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

Window::create : Memory leak on Mac OS X #484

Closed
Tokyonono opened this Issue Oct 16, 2013 · 10 comments

Comments

Projects
None yet
4 participants
@Tokyonono

Tokyonono commented Oct 16, 2013

Hi,
While checking for memory leaks, I found that calling Window:create -> Window:close -> Window::create generates some memory leaks.
I am working on Mac OS X Mountain Lion, with the latest official release of SFML (2.1).
You can see the following report for more information:
sfml memory leak
I hope it helps!
Cheers.

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Oct 16, 2013

Member

I don't fully understand the report given, but it's not really a memory leak if you just call two functions of the same class instance and more memory gets allocated with each call. It might not be optimal but as long as everything gets freed when the window gets destroyed it can't be marked as memory leak.

So how does the following code perform:

{
  sf::Window window(sf::VideoMode(1024, 768), "Test 1");
  window.close();
}
{
  sf::Window window(sf::VideoMode(1024, 768), "Test 2.1");
  window.close();
  sf::Window window(sf::VideoMode(1024, 768), "Test 2.2");
  window.close();
}
{
  sf::Window window(sf::VideoMode(1024, 768), "Test 3.1");
  window.close();
  sf::Window window(sf::VideoMode(1024, 768), "Test 3.2");
  window.close();
  sf::Window window(sf::VideoMode(1024, 768), "Test 3.3");
  window.close();
}

Also such vague issues should rather be discussed on the forum and once it's confirmed through multiple sources and made sure it's not a false positive, one can come back here. ;)

Member

eXpl0it3r commented Oct 16, 2013

I don't fully understand the report given, but it's not really a memory leak if you just call two functions of the same class instance and more memory gets allocated with each call. It might not be optimal but as long as everything gets freed when the window gets destroyed it can't be marked as memory leak.

So how does the following code perform:

{
  sf::Window window(sf::VideoMode(1024, 768), "Test 1");
  window.close();
}
{
  sf::Window window(sf::VideoMode(1024, 768), "Test 2.1");
  window.close();
  sf::Window window(sf::VideoMode(1024, 768), "Test 2.2");
  window.close();
}
{
  sf::Window window(sf::VideoMode(1024, 768), "Test 3.1");
  window.close();
  sf::Window window(sf::VideoMode(1024, 768), "Test 3.2");
  window.close();
  sf::Window window(sf::VideoMode(1024, 768), "Test 3.3");
  window.close();
}

Also such vague issues should rather be discussed on the forum and once it's confirmed through multiple sources and made sure it's not a false positive, one can come back here. ;)

@Tokyonono

This comment has been minimized.

Show comment
Hide comment
@Tokyonono

Tokyonono Oct 16, 2013

The sample code to reproduce the issue is more something like this:
{
sf::Window window;
sf::String text("TEST");
window.create(sf::VideoMode(1024, 768), text);
window.close();
window.create(sf::VideoMode(1024, 768), text);
}
You should read the "Symbol name" section of the report from top to bottom, it gives the list of calls that are made and which lead to leaks (here it leaked twice, so mind the indentation).
So according to the report, sf::priv::WindowImpl::create calls sf::priv::WindowImplCocoa::WindowImplCocoa which creates an CFString (from bytes) which is not properly destroyed.

It is not a vague issue, it is a memory leak report from XCode (which gives the point of leak), there is not much to discuss. I report it to make SFML a better product :-)

Tokyonono commented Oct 16, 2013

The sample code to reproduce the issue is more something like this:
{
sf::Window window;
sf::String text("TEST");
window.create(sf::VideoMode(1024, 768), text);
window.close();
window.create(sf::VideoMode(1024, 768), text);
}
You should read the "Symbol name" section of the report from top to bottom, it gives the list of calls that are made and which lead to leaks (here it leaked twice, so mind the indentation).
So according to the report, sf::priv::WindowImpl::create calls sf::priv::WindowImplCocoa::WindowImplCocoa which creates an CFString (from bytes) which is not properly destroyed.

It is not a vague issue, it is a memory leak report from XCode (which gives the point of leak), there is not much to discuss. I report it to make SFML a better product :-)

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini Oct 16, 2013

Member

You're talking about _256 bytes, not megabytes nor kilobytes but _bytes. Do we care that much? It's most probably a static variable somewhere..

If the leak increase each time you create a new window by an order of magnitude, ok, let's investigate. Is it the case?

Member

mantognini commented Oct 16, 2013

You're talking about _256 bytes, not megabytes nor kilobytes but _bytes. Do we care that much? It's most probably a static variable somewhere..

If the leak increase each time you create a new window by an order of magnitude, ok, let's investigate. Is it the case?

@mantognini mantognini closed this Oct 16, 2013

@Tokyonono

This comment has been minimized.

Show comment
Hide comment
@Tokyonono

Tokyonono Oct 16, 2013

Well, first of all, I had 0 leak before I started calling create()->close()->create(), which makes me think it is not a static variable somewhere, but it also increased leaks from 0 byte to 256 bytes (in the sample scenario), which is infinitely more than before, right? (division by 0)
Then, everytime I call create()->close()->create()-close()->etc. it leaks more and more. I happen to create/close/create many windows, 256 bytes can easily become megabytes, that's why we want to prevent leaks, right? If you want a report with 100Mb tell me, I can make it, but it will be a mess to read.
I wouldn't close this ticket without at least checking the part of the code which is leaking (I gave you that already, I would be thankful if you could have a look at it).
Thank you very much for your help and support, and sorry if my report bothers you, I just wanted to help.

Tokyonono commented Oct 16, 2013

Well, first of all, I had 0 leak before I started calling create()->close()->create(), which makes me think it is not a static variable somewhere, but it also increased leaks from 0 byte to 256 bytes (in the sample scenario), which is infinitely more than before, right? (division by 0)
Then, everytime I call create()->close()->create()-close()->etc. it leaks more and more. I happen to create/close/create many windows, 256 bytes can easily become megabytes, that's why we want to prevent leaks, right? If you want a report with 100Mb tell me, I can make it, but it will be a mess to read.
I wouldn't close this ticket without at least checking the part of the code which is leaking (I gave you that already, I would be thankful if you could have a look at it).
Thank you very much for your help and support, and sorry if my report bothers you, I just wanted to help.

@eXpl0it3r

This comment has been minimized.

Show comment
Hide comment
@eXpl0it3r

eXpl0it3r Oct 16, 2013

Member

For me the question still remains whether it's an actual memory leak or just a deferred release.
My posted code was not to show how I think you did it, but it was to test whether the memory is still being used after you destroy the window object completely.

As for the relevant code, given that I've no idea about OS X development, it should start here and call this, which makes me wonder whether the [NSString alloc] ever receives an release call.

Member

eXpl0it3r commented Oct 16, 2013

For me the question still remains whether it's an actual memory leak or just a deferred release.
My posted code was not to show how I think you did it, but it was to test whether the memory is still being used after you destroy the window object completely.

As for the relevant code, given that I've no idea about OS X development, it should start here and call this, which makes me wonder whether the [NSString alloc] ever receives an release call.

@Tokyonono

This comment has been minimized.

Show comment
Hide comment
@Tokyonono

Tokyonono Oct 16, 2013

@eXpl0it3r Now that you explain it, I understand your point. However, the leak is bigger when I close() and create() several times, which makes me think that the memory is actually lost and not link to the lifecycle of the window.
And you are right, sfStringToNSString(title) creates a NSString which is never released -> Memory leak.
@mantognini Source of the problem found, just needs to be fixed.

Tokyonono commented Oct 16, 2013

@eXpl0it3r Now that you explain it, I understand your point. However, the leak is bigger when I close() and create() several times, which makes me think that the memory is actually lost and not link to the lifecycle of the window.
And you are right, sfStringToNSString(title) creates a NSString which is never released -> Memory leak.
@mantognini Source of the problem found, just needs to be fixed.

@LaurentGomila

This comment has been minimized.

Show comment
Hide comment
@LaurentGomila

LaurentGomila Oct 16, 2013

Member

Marco, can't you use ARC (Automatic Reference Counting) and totally get rid of manual memory management? That's what I do for iOS.

Member

LaurentGomila commented Oct 16, 2013

Marco, can't you use ARC (Automatic Reference Counting) and totally get rid of manual memory management? That's what I do for iOS.

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini Oct 16, 2013

Member

sorry if my report bothers you, I just wanted to help.

No, no, it's always helpful and welcome! ;-)

From the previous post, it was not clear how significant the leak was. But since you spotted the right allocation I guess we have a confirmed issue.

@LaurentGomila If I drop support for 10.5 I can get some of ARC's features. With 10.7, ARC is completely supported I think.

Member

mantognini commented Oct 16, 2013

sorry if my report bothers you, I just wanted to help.

No, no, it's always helpful and welcome! ;-)

From the previous post, it was not clear how significant the leak was. But since you spotted the right allocation I guess we have a confirmed issue.

@LaurentGomila If I drop support for 10.5 I can get some of ARC's features. With 10.7, ARC is completely supported I think.

@mantognini mantognini reopened this Oct 16, 2013

@ghost ghost assigned mantognini Oct 16, 2013

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini Apr 13, 2014

Member

@Tokyonono I unsuccessfully tried to reproduce it. I know it's quite old now but do you still have the project you profiled 6 months ago? Or any project reproducing the issue – if possible with a sscce.

Member

mantognini commented Apr 13, 2014

@Tokyonono I unsuccessfully tried to reproduce it. I know it's quite old now but do you still have the project you profiled 6 months ago? Or any project reproducing the issue – if possible with a sscce.

@mantognini

This comment has been minimized.

Show comment
Hide comment
@mantognini

mantognini Apr 22, 2014

Member

I think it has been fixed when I switched to ARC.

Member

mantognini commented Apr 22, 2014

I think it has been fixed when I switched to ARC.

@mantognini mantognini closed this Apr 22, 2014

@mantognini mantognini added the resolved label Apr 22, 2014

@mantognini mantognini modified the milestones: 2.2, 2.x Apr 22, 2014

mantognini added a commit that referenced this issue May 17, 2014

Reverted OS X implementation to non-ARC
 * Apparently, there were some leaks not reported as such
 * Support for 32 bits computer is restored
 * Fix memory leak in sfStringToNSString (related to #484)
 * Unapply context when closing the window, freeing memory

The following commits are related to ARC modifications:

 * 42f6e83
 * 6edc4b9
 * f6c9445
 * 324d4a1
 * 0d47056

Commit ac28902 is the last one before the introduction of ARC.

mantognini added a commit that referenced this issue May 22, 2014

Reverted OS X implementation to non-ARC
 * Apparently, there were some leaks not reported as such
 * Support for 32 bits computer is restored
 * Fix memory leak in sfStringToNSString (related to #484)
 * Unapply context when closing the window, freeing memory

The following commits are related to ARC modifications:

 * 42f6e83
 * 6edc4b9
 * f6c9445
 * 324d4a1
 * 0d47056

Commit ac28902 is the last one before the introduction of ARC.

mantognini added a commit that referenced this issue May 23, 2014

Reverted OS X implementation to non-ARC
 * Apparently, there were some leaks not reported as such
 * Support for 32 bits computer is restored
 * Fix memory leak in sfStringToNSString (related to #484)
 * Unapply context when closing the window, freeing memory

The following commits are related to ARC modifications:

 * 42f6e83
 * 6edc4b9
 * f6c9445
 * 324d4a1
 * 0d47056

Commit ac28902 is the last one before the introduction of ARC.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment