Skip to content
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

Don't use KVO in RACCommand #2334

Merged
merged 6 commits into from Sep 12, 2015
Merged

Don't use KVO in RACCommand #2334

merged 6 commits into from Sep 12, 2015

Conversation

mdiep
Copy link
Contributor

@mdiep mdiep commented Sep 10, 2015

RACCommand has always been slow/expensive to initialize. Due to some absurd slowness in our app's unit tests, I started profiling and found that in our tests we were spending a lot of time rehashing inside KVO.

This PR changes RACCommand so that it no longer uses KVO, saving on initialization and disposal. The changes here make our app's unit tests run ~15% faster. (We create a lot of RACCommands.) I'm sure the savings won't be as noticeable in the real world, but I think these changes should still improve things.

@joshaber joshaber self-assigned this Sep 11, 2015
@joshaber
Copy link
Member

This seems legit to me, but I'd love to have a second set of 👀.

/cc @ReactiveCocoa/reactivecocoa

@neilpa
Copy link
Member

neilpa commented Sep 11, 2015

Haven't looked at the code closely yet, but this would technically be a breaking change for RAC3. This should probably be targeted at the swift2 branch for now (which I may rename RAC4 depending on the outcome of #2327).

@ashfurrow
Copy link
Member

Personally, it feels strange to remove a feature – possibly a breaking change – solely for the sake of unit test run duration.

@mdiep
Copy link
Contributor Author

mdiep commented Sep 11, 2015

How is this a breaking change? It changes the implementation, but maintains the current behavior and interface.

@ashfurrow
Copy link
Member

allowsConcurrentExecution is public property that someone may be observing. It's unlikely, given that there's a subject to use as well, but it seems like the kind of subtle change that could drive someone nuts trying to figure out why their code doesn't work anymore.

@mdiep
Copy link
Contributor Author

mdiep commented Sep 11, 2015

allowsConcurrentExecution is public property that someone may be observing. It's unlikely, given that there's a subject to use as well, but it seems like the kind of subtle change that could drive someone nuts trying to figure out why their code doesn't work anymore.

Ah, okay. I just added that back (with a test). That's not an important part of the PR—the important part is not using RACObserve internally.

Edit: Actually, that still works without changes. But there's a test now.

@ashfurrow
Copy link
Member

Coooool – looks good 💯

I'm not the most judicious code-reviewer, though. Someone else should double-check juuuust to be sure.

@neilpa
Copy link
Member

neilpa commented Sep 12, 2015

This looks right to me too so here goes nothing

neilpa added a commit that referenced this pull request Sep 12, 2015
@neilpa neilpa merged commit e16f47c into master Sep 12, 2015
@mdiep mdiep deleted the raccommand,-my-nemesis branch September 12, 2015 00:32
@mdiep
Copy link
Contributor Author

mdiep commented Sep 12, 2015

💖

@mdiep mdiep mentioned this pull request Sep 18, 2015
StatusReport added a commit to Lightricks/ReactiveCocoa that referenced this pull request Mar 31, 2016
mdiep pushed a commit that referenced this pull request May 7, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants