removeUnusedTextures in CCTextureCache is not implemented in ARC about cocos2d-objc HOT 31 CLOSED

CCSpriteFrameCache has same issue. Use a CCCache subclass to handle both/all caches?

from cocos2d-objc.

All objective-c chipmunk classes are also non-ARC, making the installation of templates a little bit more difficult than it could be (making a fat static lib, instead of copying files).

from cocos2d-objc.

Hmm. I hadn't considered the thread safety issues with retainCount. CFGetRetainCount() would have the same issues. retainCount used to be useful, but it's certainly become much less so over the years. When used from code that uses ARC, it's 100% useless since ARC may (or may not) retain local or temporary variables depending on what the compiler thinks is safe.

The NSCache seems interesting. I was unaware of that class. I'm not entirely certain what the code does though. Why is the watcher_key pointer initialized with it's own address (on the stack). Also, since the texture isn't retained anywhere, it will be deallocated the moment all of it's references are released. The watcher is only there to notify the cache when that happens right? So if you had a single sprite onscreen at any given time that used a particular texture, you might end up reloading it's texture each time the sprite was initialized.

Birkemose had an idea to use a proxy object and autozeroing weak references. If the proxy objects were deallocated, then you'd know it was safe to release a texture when a low memory event came. If you requested the texture before then, the proxy could simply be recreated.

Also, does it make sense to have different caches for different types when the keys are almost always filenames anyway? We could save a bit of complexity by just having a CCCache maybe?

As for Objective-Chipmunk, it could be placed in a separate target instead of a static lib, but it interacts with a lot of C code. Using ARC in it doesn't make a whole lot of sense.

from cocos2d-objc.

The code isn't at all complete but written by Philippe in about 5 minutes, it could serve as an idea for how to go about it. But there may be other better solutions too!

from cocos2d-objc.

@slembcke Regarding Objective-Chipmunk... well, it makes the installation of the templates much harder as I have to build the static library each time and that can easily lead to errors. And as I looked at it, it shouldn't be very hard - it is just a matter of deleting many retain/release/autorelease, fixing some structs containing objects and that's it.

from cocos2d-objc.

It is true, that Core Foundation does not apply to ARC, and as such could be used to bypass ARC. On the other hand, to me it sounds like bad design to base the texture class on CF, to avoid this. Then rather tell people they - for now - have to clean textures up manually.

OpenGL holds the texture anyways, so it would be fairly simple to create a proxy, based on this.

from cocos2d-objc.

Tested this a little bit and it seems to be a no-go unfortunately. It almost works on Mac but on iOS it returns very height (5 or 7) retain count. Need to find another solution for this.

from cocos2d-objc.

So the following does appear to work correctly, though it's a little awkward to test. It's a combination of Birkemose's and my ideas:
https://github.com/cocos2d/cocos2d-iphone/compare/unused-textures

The basic idea is that each texture has a weakly retained proxy object associated with it. The proxy object has a strong reference to the texture and forwards all method calls to it. The texture cache retains the CCTexture, but returns the proxy object. If a proxy loses all it's references, it will be deallocated instead of the actual texture. If a texture is requested from the cache, but it's proxy has been deallocated, it simply makes a new one. When a memory warning comes in, the cache looks for textures that don't have a proxy object and releases them.

I'm a little stumped how to unit test this, and since most of the existing app based tests don't compile anymore I just hacked one of them up to check it. Not very scientific...

from cocos2d-objc.

@slembcke interesting ideas, I will review this and discuss it with the other Apportable engineers tomorrow.

from cocos2d-objc.

Obviously, this is a bit of a hack. The main place where things could break, as I understand it, are places where users would call isKindOfClass. Also, there are more classes that use textures than CCSprite (e.g. CCParticleSystem), would we need to update those as well? Will users adding texture to their custom nodes take this into consideration?

I discussed this with some of the guys over here. According to them, using forwardingTargetForSelector is super slow. Not exactly a scientific statement, but it may need some further investigation/profiling before we go for this solution.

from cocos2d-objc.

• Not very much of a hack. Object substitution and duck typing is already fairly common in Objective-C. That's the entire reason for the "if((self = [super init]))" construct in initializers. Even common classes like NSString, collection classes and blocks work this way and people rarely notice.
• isKindOfClass could be overridden to be forwarded to CCTexture as well. I considered impementing it this way too, but I'm not sure if this is a more correct thing to do or not. I searched the entire project for "[CCTexture class]", and those were the only two instances. Performing class comparisons is already highly discouraged in OO programming. It's already a little odd that it was done in those two cases, since the only way to fail those two assertions is to intentionally trick the compiler by casting a non-CCTexture object as one of the arguments. (I do appreciate the irony here though...) Anybody that needs to explicitly check the class of a texture would fairly quickly figure out what was going on. An alternative solution would be to make CCTexture a protocol and have both the texture class and the proxy class be private. Then there is nothing at all to be confused about.
• forwardingTargetForSelector: is documented as being the fastest and preferred message forwarding method by Apple. The name method and also much more rarely, the width and height methods are the only ones that are called commonly at runtime. Providing concrete implementations for those methods would make the cost trivial.

from cocos2d-objc.

Philippe, my go to objective-c guru had a look at this. Posting his response below (his example is above - it's incomplete but could serve as a starting point):

forwardingTargetForSelector is not faster than forwardInvocation. it is still going down a VERY slow code path.
NSCache was built for this type of thing; I really think that the example I gave you is considerably better. If need be we can drop in our NSCache implementation and make that open source before we should even tolerate an implementation that uses forwardInvocation.

Basically any class that does not respond to the selector will end doing the following slow things: it will take the slowest objc runtime lookup code path for objc_msgSend, it then constructs and destructs an NSInvocation for each time it messages (which is about a good 10-15 extra message sends), it then hits the forwarding invoke, which uses the objc marg_list handler which has zero caching of messages, and then finally the other costly thing it will do is create and destroy a fair amount of memory that will end up fragmenting your malloc tables. This will be a considerable perf hit for something that should be inherently fast.

NSCache is not only fast, but thread safe, and manages purging for low memory notifications based on a cost system: purging the most costly items first. The basic fundamentals of having a cache is to prevent duplicate allocations and also prevent duplicate work being executed to decode from images into data/textures. Ideally the cache lookup should be cheaper for execution time than decoding the image and creating a texture. I worry that a forward based implementation will end up not only tipping the scales such that the texture creation will be faster than lookups but also that this will be a design pattern that users will think is a good practice. It is our responsibility to create something that is a mark of excellence and something that shows the best design practices; using forward invocation (even using forwardingTargetForSelector) in this case is not an appropriate usage. (the only real appropriate usage point IMHO for forwarding is for distributed objects)

from cocos2d-objc.

I'm quite certain that forwarding messages is not inherently fast using forwardingTargetForSelector:. Though as I said, since very few of the texture methods are called at anything other than initialization times, it makes little difference. The name and possibly width/height methods can receive concrete implementations that explicitly forward the call. This reduces the practical cost of the proxy to having a single extra method call as overhead.

The NSCache code above does not actually work. As it's currently written, the watchers remove the texture from the cache when it's deallocated, but nothing actually retains the texture to defer it's deallocation until the removeUnusedTextures method is called. You can only get a reference to a cached texture if there is at least one other instance of it currently in use. If you have an individual sprite that uses a particular texture, but is periodically created and destroyed, it will need to reload the texture each and every time. This is a very common use case for things like bullets, particle effects or anything else that is created on-demand. This was perhaps one of the most important feature of the original cache, and there hasn't been a solution presented to fix it for the NSCache code above.

from cocos2d-objc.

When I originally looked at this I had a very simple solution.

Would it not be possible to simply traverse the graph to grab a unique list of all the atlas texture ids currently being referenced by child nodes. Diff the result against the current texture cache and remove the unused textures.

I'm sure there may be a few caveats however this is a convenience method, personally I think you should manage your own textures anyway.

from cocos2d-objc.

To be honest, I don't think any of the examples are very good, so at this point I an tempted to try out Martins solution. As I see it, it would trash textures held manually though, which would probably have to be handled somehow.

I originally suggested the solution implemented by Scott. While I think his solution is slightly more complicated than it needs to be ( sorry Scott ), it is written in stone that a weak reference will go nil when an object is not retained anymore, and thus it can be used to detect if it is safe to delete a texture. To implement this, I only think the texture cache needs to be modified. I see no need to add code anywhere else.
Had anyone cared, I implemented this months back, when I worked on a design proposal for a new resource manager. And it works. Clean and neat.

About NSCache, I fully agree that this could be used to build our caches upon. The code above does - as Scott stated - not work to our needs, so it is irrelevant to discuss if it is of any use. If we want to base all cocos2d caches on NSCache, I suggest somebody mocks up a basic cache class, we can test on.

from cocos2d-objc.

The basic cache class (works, but needs overriding for ex. texture usage), can be reviewed here

d614473

from cocos2d-objc.

I have a fifth option, not sure this would be the way to go though, but I'm including it here for completeness when we evaluate the options we have.

The idea is to have a manual usage count, on say the CCTexture (maybe make CCTexture subclass of CCCachable). Every where the texture is used, we up the count when they are no longer used (use the dealloc method) we decrease the count. This would act similar to the retain count. When the usage count is 0, we can safely release the object.

The good: It's easy to understand and it's efficient.
The bad: We will need to modify any code uses textures. Also, anyone creating a new class that uses textures will need to understand how this works. (Although, I think this is a fairly uncommon use case.)

from cocos2d-objc.

Let's summarize the different options we have.

1. Revert to non-arc
   Pros: Easy to do
   Cons: It's better to have things consistent and all ARC, but the largest drawback is that the old code isn't actually working. It's single most common crash point in Cocos2d (according to the usage stats from Apportable customers).

2. Traverse the node graph
   Pros: Easy to implement
   Cons: Doesn't work with nodes outside the node graph. For instance if you are creating a sprite, but hasn't yet added it to the current scene and at this point get a memory warning.

3. Using forward invocation
   Pros: Quick fix, is likely to work without modifying the code.
   Cons: Extremely slow (and according to Zac & Philippe not a good solution)

4. Using ARC's nullification (Birkemose's idea)
   Pros: Will work without modifying any of the other code. Efficient.
   Cons: The data objects that are cached are not modified when properties on the actual object are modified. The data will need to be modified to contain information more than the actual texture (content size, data format etc).

5. Adding a usage count
   Pros: Easy to understand and efficient.
   Cons: Will need to modify all code that is using textures.

from cocos2d-objc.

My vote goes to 4 with some modifications or possibly 5.

For 4 maybe this could be an improvement (and make it more generic):

1. Make CCTexture conform to the copy protocol. For the actual texture data we only copy the reference. This makes a copy share the same texture data.
2. Instead of storing the sharedData in the CCCacheEntry we save a 'template' copy, and instead of calling createSharedDataForKey: we simply create a copy [_template copy].
3. We release the shared texture pointer at the time when the texture is removed from the cache.
4. To make this more generic we can make CCTexture inherit from CCCachable which have a method named "freeSharedData" which would release the shared data pointer.

from cocos2d-objc.

@Birkemose,
When you spoke of this before I either forgot that you said it was already implemented or I misunderstood. I appologize for that. I wrote the message forwarding version based on what we talked about a few weeks ago because I was unaware there was a working solution to the problem. Since it's implemented as an abstract class, it became painfully obvious to me that while we've been fervently discussing the texture cache, there are also shader, animation and sprite frame caches which all duplicate the same functionality. I also vaguely recall there being some discussion a couple months ago about having a single "asset" cache instead of caches for each individual type. I think there are some important benefits of that, but I'll create a separate discussion for that instead of derailing this one.

Birkemose's code works, and could be adapted to use a NSCache instead of a dictionary if we really wanted to use the "cost" feature. It also is a step in the direction of what to do with the other cache classes. I'm not exactly certain what ## this is in Viktor's list, but this (or a unified asset cache) makes most sense to me.

1. Using per-file compile options on files within a single target is something I like to avoid.

2. Unity seems to do something of a cross between this and #5, though it's not exactly documented. In Unity, it's impossible to have GameObject instances that aren't in the scene graph though. As Viktor pointed out, in a rare case you might end up with duplicate loaded assets because of parentless nodes outside of the scene. In an even rarer worst case, that could mean an entire duplicate texture atlas gets loaded.

3. If using concrete forwarding methods for common methods, the overhead would be practically non-existant. This could still be mixed with a generic cache implementation, but each cachable-object type would need to have the proxy and hasProxy methods added, and more "hotspot" forwarding methods may need to be added. This may or may not be simpler than implementing Birkemose's shared data/object methods, and his is already more of a generic solution.

4. I'm actually fairly confused which idea this is referring to. Birkemose's?

5. This would work, but as somebody that writes a lot of custom drawing methods, I'm biased against it for more automatic.

from cocos2d-objc.

Sorry, it was very unclear. Number 4 is Birkemose's idea.

from cocos2d-objc.

Is it just me, or does any else having troubles keeping up with the numbers? Let me add some letters ;)
About your points Viktor, let me see if I got it right.
a)
I am not sure what you mean by "The data objects that are cached are not modified when properties on the actual object are modified".
The object cached should be an object as any other object, so I see no reason why modifying its properties, should not reflect on the object. Am I missing something here?
b)
I am not 100% sure I understand the point about not saving sharedData. In the case where an object is not used anymore, and the pointer goes nil, the sharedData is all there is to recreate a new object. As the cache is meant to be generic, this could be a GLKTextureInfo, or it could be an allocated chunk of audio.
But all this copy protocol stuff, is not my strongest point.

from cocos2d-objc.

Bill mentioned this in an email and I've been thinking about it too. Are people opposed to hiding the various caches behind class methods like [UIImage imageNamed:] does?

Like [CCTexture textureWithImageNamed:] and an async variant could be added that would check the cache and return the cached or newly loaded texture. I would bet that 99% of the time, that's all people want when they interact with the cache anyway. The CGImage and PVR methods of CCTextureCache are probably very rarely used.

from cocos2d-objc.

I vote for hiding the cache, but that is actually not my main problem in this.
Problem is, that we should either just do a simple fix to support the retain count flushing, in which case my cache class should be hacked to support that, or we should scrap everything, and just go with GLKTextureLoader ... which will be the final solution anyways.

from cocos2d-objc.

@slembcke

Separate async versions are not needed for any of this, and create unnecessary conceptual confusion for the developers.

Loading textures and placing into the (hidden) cache can be done on the background thread using the same API.

It's when it gets attached to a node that it become important to move to the rendering thread. That is a different task than loading and caching.

from cocos2d-objc.

@Birkemose

GLKTextureLoader is iOS 5.0 and above.

Is the minimum target OS for cocos2d 3.0 iOS 5.0?

from cocos2d-objc.

For what it's worth...

cocos3d's approach to auto-clearing the cache is to design the cache to hold either or both strong and weak references to the textures. The developer chooses whether a texture should be strongly or weakly cached when it is loaded.

In concert with this, the texture class dealloc method implements a call to the cache instance, to remove itself from the cache.

So...

• A strongly-cached texture never gets removed, because it is never deallocated, because it is strongly held by the cache itself.
• A weakly cached texture will be deallocated when it's not being used, and will automatically remove itself from the cache when that happens.
• No graph traversals are required.

from cocos2d-objc.

@Bill, yeah, iOS5 is now entry level, so it is only a matter of time until we rip out all texture and cache code, and replace it all with 10 lines of GLKTextureLoader.

The problem is, that the cocos2d cache does not work that way. It keeps all textures in memory, until the user specifically flushed the unused ones, or when a memory warning occurs.
I personally think it should continue to be that way.

All in all, the changes we need to do to all this, seems to be too big for a V3 release, so I will probably just add a "hack" to the cache, and leave everything as it is now.

from cocos2d-objc.

+1 hiding the cache.

GLKTextureLoader sounds like a nice way to refactor all that Texture loading.

from cocos2d-objc.

So looks like we are going to go with the proxy solution for now since it requires minimal changes. Other than a bug with [CCTexture createSpriteFrame] creating a spriteFrame with an explicit instead of proxy reference, it seems to be working as intended now. I added a few concrete forwarding methods for anything used at runtime, when creating sprites or when creating sprite frames so the proxies should have little to no measurable cost.

Should we close this issue and start a new one in a few weeks when we figure out what to do for texture loading changes in 3.1?

from cocos2d-objc.

The initial solution for adding generic caching, has been postponed, as it required way to many changes to the existing code. In stead, Scott's solution has been chosen (at least temporarily), as it is very easy to add to existing code.
I have tested the cache, and it seems to work fine.

from cocos2d-objc.