macOS Cocoa: Am I creating a memory leak here?

[Soulstorm]

Am I creating a memory leak here?

I have a Cocoa Application and I have set a table view to hold a class (whose contents are written to the hard disk using the "createFileFromSourceDataAtPath" function of the class.

I believe I am creating a memory leak inside the 'for' loop.

Does anyone know how can this be made better? You don't have to see the entire program I think, but if you want, tell me so.

-(IBAction)exportSelection:(id)sender
{
	unsigned int i;
	NSIndexSet *indexSet = [tableView selectedRowIndexes];
	NSString *outputDirectory = [exportText stringValue];
	
	printf("output directory: %s",[outputDirectory cStringUsingEncoding:NSUTF8StringEncoding]);
	
	for(i=[indexSet firstIndex]; (i!=NSNotFound); i=[indexSet indexGreaterThanIndex:i]){
		NSString *outputFile = [outputDirectory stringByAppendingPathComponent:[[dataContainer objectAtIndex:i]originalName]];
		[[dataContainer objectAtIndex:i]createFileFromSourceDataAtPath:outputFile keepOriginalExtension:NO];
	}
	
}

The NSString 'outputFile' is not released inside the for loop... I think it's a memory leak but I'm not sure. Also, the 'outputDirectory' is not released inside the function. Is this too a memory leak? To rectify the possibility of a memory leak, I have tried to add '[outputFile release]' at the end of the 'for' loop, but that caused the debugger to show up and present some errors that I can't explain. Same happened when I tried to put '[outputFile release]' after the loop was over.

What can I do?

 

[robbieduncan]

As the NSString outputFile is not created by alloc/init or copy then it should be, by convention, autoreleased so no worries

 

[HiRez]

Yeah, looks OK. As Robbie said, those particular NSStrings should be autoreleased, so the default autorelease pool will deallocate them automatically after they go out of scope.

 

[whooleytoo]

Yeah, looks OK. As Robbie said, those particular NSStrings should be autoreleased, so the default autorelease pool will deallocate them automatically after they go out of scope.

This is something I've often been curious about - the autorelease pool doesn't release the objects until later (sometime outside the scope of the method listed above), but the method doesn't keep any reference to them. So, am I right in assuming the autorelease pool must keep its own private reference to all autoreleased objects, so as to release them later?

 

[Soulstorm]

Yeah, looks OK. As Robbie said, those particular NSStrings should be autoreleased, so the default autorelease pool will deallocate them automatically after they go out of scope.

Hm... I have 2 questions here:

In my program I haven't created an autorelease pool. My cocoa main.m looks like this:

int main(int argc, char *argv[])
{
    return NSApplicationMain(argc,  (const char **) argv);
}

Where must I put the autorelease pool? Should I create an autorelease pool inside the function I gave you and release the pool at the end of it?

 

[whooleytoo]

Hm... I have 2 questions here:

In my program I haven't created an autorelease pool. My cocoa main.m looks like this:

int main(int argc, char *argv[])
{
    return NSApplicationMain(argc,  (const char **) argv);
}

Where must I put the autorelease pool? Should I create an autorelease pool inside the function I gave you and release the pool at the end of it?

You don't need to; unless you create new threads, or have a loop where you're creating a large number of autoreleased objects. One will be created for you.

 

[Soulstorm]

OK thanks everyone, but I have one last question. When I create those objects, I take memory space. These objects will normally be released with the next pool release. But that means that until the next pool release, (which normally is at the end of the application, unless I specify otherwise) those objects still preserve this space.

Isn't that wrong? Should I find a way to deallocate those objects?

 

[whooleytoo]

OK thanks everyone, but I have one last question. When I create those objects, I take memory space. These objects will normally be released with the next pool release. But that means that until the next pool release, (which normally is at the end of the application, unless I specify otherwise) those objects still preserve this space.

Isn't that wrong? Should I find a way to deallocate those objects?

It's not wrong, but it is better to release them. Try:

for(i=[indexSet firstIndex]; (i!=NSNotFound); i=[indexSet indexGreaterThanIndex:i])
{
                NSAutoreleasePool* pool = [[NSAutoreleasePool alloc] init] ;

                NSString *outputFile = [outputDirectory stringByAppendingPathComponent:[[dataContainer objectAtIndex:i]originalName]];
                [[dataContainer objectAtIndex:i]createFileFromSourceDataAtPath:outputFile keepOriginalExtension:NO];

                [pool release] ;
 }

That'll free the string each time through the loop - it's a good idea if the loop is likely to run many times.

Incidentally, it's best not to assume the autorelease pool only releases when the program terminates, it could happen more often. It's best to assume it happens 'sometime outside the current scope'.

 

[savar]

This is something I've often been curious about - the autorelease pool doesn't release the objects until later (sometime outside the scope of the method listed above), but the method doesn't keep any reference to them. So, am I right in assuming the autorelease pool must keep its own private reference to all autoreleased objects, so as to release them later?

That's exactly how it works. In fact, the default pool always releases objects outside of the scope of the method in which they were defined, because the autorelease pool is called by the run loop of NSApplication, and you can't call that in the same thread if you're still inside your method. I don't know the actual implementation Apple uses, but you could write a custom autorelease class by using an NSMutableArray, then sending a release message to all objects in the pool, then releasing the array. (The array sends its own retain, so each object has 2 retains and must be released twice.)

Don't worry about freeing them yourself unless you're creating and destroying lots of objects in a very short time. (If you are doing this, you might want to re-engineer your program to not create so many objects.) In most cases, the default autorelease pool suffices.

 

[HiRez]

So, am I right in assuming the autorelease pool must keep its own private reference to all autoreleased objects, so as to release them later?

Yep.

OK thanks everyone, but I have one last question. When I create those objects, I take memory space. These objects will normally be released with the next pool release. But that means that until the next pool release, (which normally is at the end of the application, unless I specify otherwise) those objects still preserve this space.

Isn't that wrong? Should I find a way to deallocate those objects?

The objects the pool tracks are released periodically, not just when the pool itself is released at program termination. Specifically, as I understand it, once each pass through the event loop (tracking mouse clicks, keyboard presses, network i/o, etc.), which is pretty frequently, so normally you don't need to worry about unfreed memory from autoreleases accumulating on you. As Savar said, if you're creating and autoreleasing thousands of objects in a tight loop or function that gets called often, you might want to think about setting up a local autorelease pool, but it is rarely necessary.

 

[Soulstorm]

OK thanks a lot!

 

[HiRez]

if you're creating and autoreleasing thousands of objects in a tight loop or function that gets called often, you might want to think about setting up a local autorelease pool

Actually, now that I think about it, if you're creating and destroying fewer very large objects like images, or large buffers, you might want to do the same.

 

[Krevnik]

It's not wrong, but it is better to release them. Try:

for(i=[indexSet firstIndex]; (i!=NSNotFound); i=[indexSet indexGreaterThanIndex:i])
{
                NSAutoreleasePool* pool = [[NSAutoreleasePool alloc] init] ;

                NSString *outputFile = [outputDirectory stringByAppendingPathComponent:[[dataContainer objectAtIndex:i]originalName]];
                [[dataContainer objectAtIndex:i]createFileFromSourceDataAtPath:outputFile keepOriginalExtension:NO];

                [pool release] ;
 }

That'll free the string each time through the loop - it's a good idea if the loop is likely to run many times.

Incidentally, it's best not to assume the autorelease pool only releases when the program terminates, it could happen more often. It's best to assume it happens 'sometime outside the current scope'.

That code is pretty quickly written. Performance would suffer pretty fast with that loop.

Personally, I would take the route of allocating and releasing this pool outside the loop instead of inside. That way you have the allocation overhead of n+1 for the pool instead of 2n. Really only when you are talking about a selection set that could reach 4k strings would I run that within the loop, and then it would be periodically every X iterations in the loop, releasing in chunks to prevent a 1MB RAM spike.

 

[whooleytoo]

Personally, I would take the route of allocating and releasing this pool outside the loop instead of inside. That way you have the allocation overhead of n+1 for the pool instead of 2n. Really only when you are talking about a selection set that could reach 4k strings would I run that within the loop, and then it would be periodically every X iterations in the loop, releasing in chunks to prevent a 1MB RAM spike.

Without knowing what temporary objects are created in the method createFileFromSourceDataAtPath:keepOriginalExtension:, we can't really tell how quickly memory usage would grow with each iteration through the loop. And, we don't know how many iterations (files in that directory) there are likely to be.

Hence, here I deliberately coded for a worst case scenario, and release the pool each iteration. As you rightly say, creating and releasing the pool outside the loop, and releasing every X iterations is certainly more efficient, but even then then, 'X' would depend on the details mentioned above; and probably would have over-complicated the answer in this case.

 

[Soulstorm]

whooleytoo's method was better. Inside each loop, much data is created, and also, the loop sometimes will reach being repeated 1000 times. I too thought of releasing the pool after the end of the loop, but it seemed better to release it each time.