Skip to content
This repository has been archived by the owner on Jun 2, 2018. It is now read-only.

NSObject+BlockObservation.m: -dealloc swizzling should be smarter to avoid redundant swizzling #185

Closed
ipmcc opened this issue Oct 16, 2013 · 2 comments
Assignees
Milestone

Comments

@ipmcc
Copy link
Contributor

ipmcc commented Oct 16, 2013

In the current implementation, if you have a hierarchy like this:

NSObject (implements dealloc) -> Foo (implements dealloc) -> Bar (doesn't implement dealloc)

the current dealloc swizzling will wrap Foo's dealloc, then wrap it again when Bar is swizzled, because the check is by classname. This will mean that when a Bar is dealloced, removeAllBlockObservers will be called twice. This isn't the end of the world since -removeAllBlockObservers appears idempotent, but it does take a lock (because of @synchronized), so it's not free.

A better check would be to keep track of the pointers to all the -dealloc IMPs that bk_addObserverForKeyPaths generated, and then only swizzle if the current IMP is not in that set. (i.e. disregard the classname, and look only at the implementation.)

Incidentally, it's probably not a big deal, but -removeAllBlockObservers could probably avoid taking that lock when called in dealloc if it had an argument that told it that it was in dealloc.

@ghost ghost assigned zwaldowski Oct 17, 2013
@zwaldowski
Copy link
Collaborator

The swizzled dealloc doesn't call removeAllBlockObservers, it calls bk_removeAllAssociatedObjects, which saves the lock you mention, however _BKObserver could and should not @synchronized during dealloc so that will be changed! Thanks.

@zwaldowski
Copy link
Collaborator

The way I describe it is as it is today in next. Sorry about the confusion I might've caused.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants