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

Moderately complex signal performance #646

Closed
epatey opened this Issue Jul 3, 2013 · 11 comments

Comments

Projects
None yet
5 participants
@epatey
Contributor

epatey commented Jul 3, 2013

I LOVE Reactive Cocoa. Thank you!

Over the last month or so, I've been really expanding my RAC usage, but I think I've hit a practical limit. It seems the sweetness of the reactive pattern carries a perf cost of about an order of magnitude over, for example, addObserver:forKeyPath:.

When using RAC within a UITableViewCell, this cost can easily become high enough to make scrolling the table view very choppy.

I wrote some code, included below, that iterates 100 times subscribing to a simple combineNext: of 4 signals and then does similar operation using addObserver: When I run the sample, I get the following output.

RAC took [548.509 ms]
Native took [56.558 ms]

This says that subscribing to a combineLatest: of four signals takes about 5.5ms as opposed to .57ms to observe 4 properties natively.

In my real code, my cell's complexity requires the composition of many more signals pushing the per cell setup time to 10's of ms - well above the choppiness threshold.

Have other people encountered this?

BTW, to get around this, I've had to maintain both imperative and declarative approaches in my code. When the cell loads, I update my UI imperatively. Only if the cell remains visible for ~1s, do I then set up the reactive stuff so that the cell refreshes dynamically. Obviously, even though this solves my problem, it's a real bummer.

@property (readonly, nonatomic) BOOL test;
...
    for (int i = 0; i< 100; i++) {
        RACSignal *sig = RACAbleWithStart(self, test);
        [[[RACSignal combineLatest:@[sig, sig, sig, sig]] deliverOn:[RACScheduler mainThreadScheduler]] subscribeNext:^(id x) {
        }];
    }

    [timer capture];
    NSLog(@"RAC took %@", [timer stringWithElapsedTime]);
    [timer start];

    for (int i = 0; i< 100; i++) {
        [self addObserver:self forKeyPath:@"test" options:0 context:nil];
        [self addObserver:self forKeyPath:@"test" options:0 context:nil];
        [self addObserver:self forKeyPath:@"test" options:0 context:nil];
        [self addObserver:self forKeyPath:@"test" options:0 context:nil];
    }

    [timer capture];
    NSLog(@"Native took %@", [timer stringWithElapsedTime]);
    [timer start];

EDIT: FYI, these times are from an iPhone 5 device. The problem is obviously worse on single core devices.

@BCeprice

This comment has been minimized.

Show comment
Hide comment
@BCeprice

BCeprice Jul 3, 2013

KVO doesn't funnel through a dispatch queue. What happens if you use
+immediateScheduler instead?

e

On Tuesday, July 2, 2013, Eric Patey wrote:

I LOVE Reactive Cocoa. Thank you!

Over the last month or so, I've been really expanding my RAC usage, but I
think I've recently hit a practical limit. It seems the sweetness of the
reactive pattern carries a perf cost of about an order of magnitude over,
for example, addObserver:forKeyPath:.

When using RAC within a UITableViewCell, this cost can easily become high
enough to make scrolling the table view very choppy.

I wrote some code, included below, that iterates 100 times subscribing to
a simple combineNext: of 4 signals and then does similar operation using
addObserver: When I run the sample, I get the following output.

RAC took [548.509 ms]
Native took [56.558 ms]

This says that simply subscribing to a combineLatest: of four signals
takes about 5.4ms as opposed to .57ms to simply observe 4 properties
natively.

In my real code, my cell's complexity requires the composition of many
more signals pushing the per cell setup time to 10's of ms - well above the
choppiness threshold.

Have other people encountered this?

BTW, to get around this, I've had to maintain both imperative and
declarative approaches in my code. When the cell loads, I update my UI
imperatively. Only if the cell remains visible for ~1s, do I then set up
the reactive stuff so that the cell refreshes dynamically. Obviously, event
though this solves my problem, it's a real bummer.

@Property (readonly, nonatomic) BOOL test;...
for (int i = 0; i< 100; i++) {
RACSignal *sig = RACAbleWithStart(self, test);
[[[RACSignal combineLatest:@[sig, sig, sig, sig]] deliverOn:[RACScheduler mainThreadScheduler]] subscribeNext:^(id x) {
}];
}

[timer capture];
NSLog(@"RAC took %@", [timer stringWithElapsedTime]);
[timer start];

for (int i = 0; i< 100; i++) {
    [self addObserver:self forKeyPath:@"test" options:0 context:nil];
    [self addObserver:self forKeyPath:@"test" options:0 context:nil];
    [self addObserver:self forKeyPath:@"test" options:0 context:nil];
    [self addObserver:self forKeyPath:@"test" options:0 context:nil];
}

[timer capture];
NSLog(@"Native took %@", [timer stringWithElapsedTime]);
[timer start];


Reply to this email directly or view it on GitHubhttps://github.com/ReactiveCocoa/ReactiveCocoa/issues/646
.

BCeprice commented Jul 3, 2013

KVO doesn't funnel through a dispatch queue. What happens if you use
+immediateScheduler instead?

e

On Tuesday, July 2, 2013, Eric Patey wrote:

I LOVE Reactive Cocoa. Thank you!

Over the last month or so, I've been really expanding my RAC usage, but I
think I've recently hit a practical limit. It seems the sweetness of the
reactive pattern carries a perf cost of about an order of magnitude over,
for example, addObserver:forKeyPath:.

When using RAC within a UITableViewCell, this cost can easily become high
enough to make scrolling the table view very choppy.

I wrote some code, included below, that iterates 100 times subscribing to
a simple combineNext: of 4 signals and then does similar operation using
addObserver: When I run the sample, I get the following output.

RAC took [548.509 ms]
Native took [56.558 ms]

This says that simply subscribing to a combineLatest: of four signals
takes about 5.4ms as opposed to .57ms to simply observe 4 properties
natively.

In my real code, my cell's complexity requires the composition of many
more signals pushing the per cell setup time to 10's of ms - well above the
choppiness threshold.

Have other people encountered this?

BTW, to get around this, I've had to maintain both imperative and
declarative approaches in my code. When the cell loads, I update my UI
imperatively. Only if the cell remains visible for ~1s, do I then set up
the reactive stuff so that the cell refreshes dynamically. Obviously, event
though this solves my problem, it's a real bummer.

@Property (readonly, nonatomic) BOOL test;...
for (int i = 0; i< 100; i++) {
RACSignal *sig = RACAbleWithStart(self, test);
[[[RACSignal combineLatest:@[sig, sig, sig, sig]] deliverOn:[RACScheduler mainThreadScheduler]] subscribeNext:^(id x) {
}];
}

[timer capture];
NSLog(@"RAC took %@", [timer stringWithElapsedTime]);
[timer start];

for (int i = 0; i< 100; i++) {
    [self addObserver:self forKeyPath:@"test" options:0 context:nil];
    [self addObserver:self forKeyPath:@"test" options:0 context:nil];
    [self addObserver:self forKeyPath:@"test" options:0 context:nil];
    [self addObserver:self forKeyPath:@"test" options:0 context:nil];
}

[timer capture];
NSLog(@"Native took %@", [timer stringWithElapsedTime]);
[timer start];


Reply to this email directly or view it on GitHubhttps://github.com/ReactiveCocoa/ReactiveCocoa/issues/646
.

@notxcain

This comment has been minimized.

Show comment
Hide comment
@notxcain

notxcain Jul 3, 2013

I've encountered performance issue when was creating form validity signal combined from 14 field validity signals.

- (void)bindValidProperty
{
    [self.validationDisposable dispose];

    RACSequence *validitySignals = [self.visibleFIelds map:^id (DFField *field) {
        return [RACAbleWithStart(field, valid) distinctUntilChanged];
    }];

    RACSignal *validationSignal = [[[RACSignal combineLatest:validitySignals] map:^id (RACTuple *tuple) {
        return @(![[tuple allObjects] containsObject:@NO]);
    }] distinctUntilChanged];

    self.validationDisposable = [validationSignal subscribeNext:^(NSNumber *formIsValid) {
        self.valid = [formIsValid boolValue];
    }];
}

notxcain commented Jul 3, 2013

I've encountered performance issue when was creating form validity signal combined from 14 field validity signals.

- (void)bindValidProperty
{
    [self.validationDisposable dispose];

    RACSequence *validitySignals = [self.visibleFIelds map:^id (DFField *field) {
        return [RACAbleWithStart(field, valid) distinctUntilChanged];
    }];

    RACSignal *validationSignal = [[[RACSignal combineLatest:validitySignals] map:^id (RACTuple *tuple) {
        return @(![[tuple allObjects] containsObject:@NO]);
    }] distinctUntilChanged];

    self.validationDisposable = [validationSignal subscribeNext:^(NSNumber *formIsValid) {
        self.valid = [formIsValid boolValue];
    }];
}
@BCeprice

This comment has been minimized.

Show comment
Hide comment
@BCeprice

BCeprice Jul 3, 2013

My apologies, I see that you are testing subscriptions, not sends. Please
disregard.

e

On Wednesday, July 3, 2013, Erik Price wrote:

KVO doesn't funnel through a dispatch queue. What happens if you use
+immediateScheduler instead?

e

On Tuesday, July 2, 2013, Eric Patey wrote:

I LOVE Reactive Cocoa. Thank you!

Over the last month or so, I've been really expanding my RAC usage, but I
think I've recently hit a practical limit. It seems the sweetness of the
reactive pattern carries a perf cost of about an order of magnitude over,
for example, addObserver:forKeyPath:.

When using RAC within a UITableViewCell, this cost can easily become high
enough to make scrolling the table view very choppy.

I wrote some code, included below, that iterates 100 times subscribing to
a simple combineNext: of 4 signals and then does similar operation using
addObserver: When I run the sample, I get the following output.

RAC took [548.509 ms]
Native took [56.558 ms]

This says that simply subscribing to a combineLatest: of four signals
takes about 5.4ms as opposed to .57ms to simply observe 4 properties
natively.

In my real code, my cell's complexity requires the composition of many
more signals pushing the per cell setup time to 10's of ms - well above the
choppiness threshold.

Have other people encountered this?

BTW, to get around this, I've had to maintain both imperative and
declarative approaches in my code. When the cell loads, I update my UI
imperatively. Only if the cell remains visible for ~1s, do I then set up
the reactive stuff so that the cell refreshes dynamically. Obviously, event
though this solves my problem, it's a real bummer.

@Property (readonly, nonatomic) BOOL test;...
for (int i = 0; i< 100; i++) {
RACSignal *sig = RACAbleWithStart(self, test);
[[[RACSignal combineLatest:@[sig, sig, sig, sig]] deliverOn:[RACScheduler mainThreadScheduler]] subscribeNext:^(id x) {
}];
}

[timer capture];
NSLog(@"RAC took %@", [timer stringWithElapsedTime]);
[timer start];

for (int i = 0; i< 100; i++) {
    [self addObserver:self forKeyPath:@"test" options:0 context:nil];
    [self addObserver:self forKeyPath:@"test" options:0 context:nil];
    [self addObserver:self forKeyPath:@"test" options:0 context:nil];
    [self addObserver:self forKeyPath:@"test" options:0 context:nil];
}

[timer capture];
NSLog(@"Native took %@", [timer stringWithElapsedTime]);
[timer start];


Reply to this email directly or view it on GitHubhttps://github.com/ReactiveCocoa/ReactiveCocoa/issues/646
.

BCeprice commented Jul 3, 2013

My apologies, I see that you are testing subscriptions, not sends. Please
disregard.

e

On Wednesday, July 3, 2013, Erik Price wrote:

KVO doesn't funnel through a dispatch queue. What happens if you use
+immediateScheduler instead?

e

On Tuesday, July 2, 2013, Eric Patey wrote:

I LOVE Reactive Cocoa. Thank you!

Over the last month or so, I've been really expanding my RAC usage, but I
think I've recently hit a practical limit. It seems the sweetness of the
reactive pattern carries a perf cost of about an order of magnitude over,
for example, addObserver:forKeyPath:.

When using RAC within a UITableViewCell, this cost can easily become high
enough to make scrolling the table view very choppy.

I wrote some code, included below, that iterates 100 times subscribing to
a simple combineNext: of 4 signals and then does similar operation using
addObserver: When I run the sample, I get the following output.

RAC took [548.509 ms]
Native took [56.558 ms]

This says that simply subscribing to a combineLatest: of four signals
takes about 5.4ms as opposed to .57ms to simply observe 4 properties
natively.

In my real code, my cell's complexity requires the composition of many
more signals pushing the per cell setup time to 10's of ms - well above the
choppiness threshold.

Have other people encountered this?

BTW, to get around this, I've had to maintain both imperative and
declarative approaches in my code. When the cell loads, I update my UI
imperatively. Only if the cell remains visible for ~1s, do I then set up
the reactive stuff so that the cell refreshes dynamically. Obviously, event
though this solves my problem, it's a real bummer.

@Property (readonly, nonatomic) BOOL test;...
for (int i = 0; i< 100; i++) {
RACSignal *sig = RACAbleWithStart(self, test);
[[[RACSignal combineLatest:@[sig, sig, sig, sig]] deliverOn:[RACScheduler mainThreadScheduler]] subscribeNext:^(id x) {
}];
}

[timer capture];
NSLog(@"RAC took %@", [timer stringWithElapsedTime]);
[timer start];

for (int i = 0; i< 100; i++) {
    [self addObserver:self forKeyPath:@"test" options:0 context:nil];
    [self addObserver:self forKeyPath:@"test" options:0 context:nil];
    [self addObserver:self forKeyPath:@"test" options:0 context:nil];
    [self addObserver:self forKeyPath:@"test" options:0 context:nil];
}

[timer capture];
NSLog(@"Native took %@", [timer stringWithElapsedTime]);
[timer start];


Reply to this email directly or view it on GitHubhttps://github.com/ReactiveCocoa/ReactiveCocoa/issues/646
.

@jspahrsummers

This comment has been minimized.

Show comment
Hide comment
@jspahrsummers

jspahrsummers Jul 3, 2013

Member

@epatey What is the real RAC code you're using? Perhaps we can help write a more optimized version.

Failing that, can you grab any Instruments traces and upload them somewhere? We can't really improve the performance of the framework without knowing where the hotspots are.

@denis-mikhaylov What are the performance issues you encountered? Did you profile it with Instruments?

Member

jspahrsummers commented Jul 3, 2013

@epatey What is the real RAC code you're using? Perhaps we can help write a more optimized version.

Failing that, can you grab any Instruments traces and upload them somewhere? We can't really improve the performance of the framework without knowing where the hotspots are.

@denis-mikhaylov What are the performance issues you encountered? Did you profile it with Instruments?

@epatey

This comment has been minimized.

Show comment
Hide comment
@epatey

epatey Jul 3, 2013

Contributor

Thanks for the response.

Yes, I've run instruments. Here's the trace: https://docs.google.com/file/d/0B1Fj9sRUmbRVNnZkMTE5cXZxQ28/edit?usp=sharing

I've selected the spike at 20 secs. That's when I ran my sample code.

My real subscription is essentially the same. I combineLatest: several properties on another object using RACAbleWithStart. The properties to which I subscribe are simple properties backed by ivars.

Unfortunately, I couldn't find any smoking guns in the trace. In other words, it looked like no particular method was taking a large amount of time. Rather, it is simply the very large set of methods being called, each of which does a small amount of work.

I wonder if a solution would be a less recursive implementation of combineLatest:. It would definitely be less slick, but I imagine it could dramatically reduce the number of subscriptions, disposables, etc.

Contributor

epatey commented Jul 3, 2013

Thanks for the response.

Yes, I've run instruments. Here's the trace: https://docs.google.com/file/d/0B1Fj9sRUmbRVNnZkMTE5cXZxQ28/edit?usp=sharing

I've selected the spike at 20 secs. That's when I ran my sample code.

My real subscription is essentially the same. I combineLatest: several properties on another object using RACAbleWithStart. The properties to which I subscribe are simple properties backed by ivars.

Unfortunately, I couldn't find any smoking guns in the trace. In other words, it looked like no particular method was taking a large amount of time. Rather, it is simply the very large set of methods being called, each of which does a small amount of work.

I wonder if a solution would be a less recursive implementation of combineLatest:. It would definitely be less slick, but I imagine it could dramatically reduce the number of subscriptions, disposables, etc.

@jspahrsummers

This comment has been minimized.

Show comment
Hide comment
@jspahrsummers

jspahrsummers Jul 3, 2013

Member

@epatey Looks like that file got corrupted/truncated. Can you try zipping then uploading it again?

I combineLatest: several properties on another object using RACAbleWithStart. The properties to which I subscribe are simple properties backed by ivars.

Do you actually need the combined values of all those properties? If not, +merge: might be more performant.

Member

jspahrsummers commented Jul 3, 2013

@epatey Looks like that file got corrupted/truncated. Can you try zipping then uploading it again?

I combineLatest: several properties on another object using RACAbleWithStart. The properties to which I subscribe are simple properties backed by ivars.

Do you actually need the combined values of all those properties? If not, +merge: might be more performant.

@epatey

This comment has been minimized.

Show comment
Hide comment
@epatey

epatey Jul 3, 2013

Contributor

Sorry...here's the zipped one.

https://docs.google.com/file/d/0B1Fj9sRUmbRVTVR1enZnRUpLTDA/edit?usp=sharing

Sounds like you're suggesting I could use merge and treat it just as a trigger to refresh my UI as long as I have a way to go get the current value of all of the properties rather than relying upon combine to deliver all of the current values. Yes?

Contributor

epatey commented Jul 3, 2013

Sorry...here's the zipped one.

https://docs.google.com/file/d/0B1Fj9sRUmbRVTVR1enZnRUpLTDA/edit?usp=sharing

Sounds like you're suggesting I could use merge and treat it just as a trigger to refresh my UI as long as I have a way to go get the current value of all of the properties rather than relying upon combine to deliver all of the current values. Yes?

@jspahrsummers

This comment has been minimized.

Show comment
Hide comment
@jspahrsummers

jspahrsummers Jul 3, 2013

Member

Nah, sorry, I meant if you don't need all of the current values, a +merge: would be faster. But if you do, +combineLatest: is a good choice, and we should focus on optimizing it.

Member

jspahrsummers commented Jul 3, 2013

Nah, sorry, I meant if you don't need all of the current values, a +merge: would be faster. But if you do, +combineLatest: is a good choice, and we should focus on optimizing it.

@jspahrsummers

This comment has been minimized.

Show comment
Hide comment
@jspahrsummers

jspahrsummers Jul 3, 2013

Member

@epatey So, after looking at that trace, I have a few concrete suggestions:

  1. Synthesize an atomic observationInfo property, to reduce the cost of setting up/tearing down KVO observations.
  2. If the properties will only change on the main thread, eliminate the -deliverOn:. It skips a run loop iteration unnecessarily, and results in additional subscription overhead.
  3. If you don't need the properties' initial values, use RACAble or -startWith:. The KVO comparison isn't exactly the same in this regard because it doesn't use NSKeyValueObservingOptionInitial.

It also looks like you're running this in DEBUG, where RAC skips a few optimizations to increase debuggability. A comparison of the relative performance on Release would be more accurate.

Let me know if this helps. 🌠

Member

jspahrsummers commented Jul 3, 2013

@epatey So, after looking at that trace, I have a few concrete suggestions:

  1. Synthesize an atomic observationInfo property, to reduce the cost of setting up/tearing down KVO observations.
  2. If the properties will only change on the main thread, eliminate the -deliverOn:. It skips a run loop iteration unnecessarily, and results in additional subscription overhead.
  3. If you don't need the properties' initial values, use RACAble or -startWith:. The KVO comparison isn't exactly the same in this regard because it doesn't use NSKeyValueObservingOptionInitial.

It also looks like you're running this in DEBUG, where RAC skips a few optimizations to increase debuggability. A comparison of the relative performance on Release would be more accurate.

Let me know if this helps. 🌠

@epatey

This comment has been minimized.

Show comment
Hide comment
@epatey

epatey Jul 3, 2013

Contributor

Thanks for all of the suggestions.

  1. I'll try this in bit. I fear that the actually cost of creating the observation is relatively small though.
  2. I understand. If I'm guaranteed to have the signal fire immediately, delivering it async just adds more work.
  3. I do need the initial values so that I can set by UI properly based on those values. I get your point about NSKeyValueObservingOptionInitial, but even when switching to RACAble, though, it only makes a tiny difference.

I'll measure in release instead of debug.

At the end of the day, though, I fear the current approach isn't within striking distance for use UITableViewCell use cases.

Do you have any reaction to my thought about a non-recursive combineLatest: implementation? Is that naive?

Contributor

epatey commented Jul 3, 2013

Thanks for all of the suggestions.

  1. I'll try this in bit. I fear that the actually cost of creating the observation is relatively small though.
  2. I understand. If I'm guaranteed to have the signal fire immediately, delivering it async just adds more work.
  3. I do need the initial values so that I can set by UI properly based on those values. I get your point about NSKeyValueObservingOptionInitial, but even when switching to RACAble, though, it only makes a tiny difference.

I'll measure in release instead of debug.

At the end of the day, though, I fear the current approach isn't within striking distance for use UITableViewCell use cases.

Do you have any reaction to my thought about a non-recursive combineLatest: implementation? Is that naive?

@jspahrsummers

This comment has been minimized.

Show comment
Hide comment
@jspahrsummers

jspahrsummers Jul 5, 2013

Member

I fear that the actually cost of creating the observation is relatively small though.

The trace you sent included at least ~50ms setting up the observation. Any time saved should be valuable for the case you're describing.

Do you have any reaction to my thought about a non-recursive combineLatest: implementation? Is that naive?

No, it's not naive — in fact, that's how it was implemented originally — but it's a lot more work to maintain two completely separate methods that do the same thing.

However, if you have any ideas for simplifying the implementation of +combineLatest: while still calling through to +combineLatestWith:, we'd be happy to review a PR.

In the end, though, there will unfortunately be some cases for which RAC just isn't fast enough. We'd love to optimize wherever we can, but single-threaded imperative code will almost always be faster.

Member

jspahrsummers commented Jul 5, 2013

I fear that the actually cost of creating the observation is relatively small though.

The trace you sent included at least ~50ms setting up the observation. Any time saved should be valuable for the case you're describing.

Do you have any reaction to my thought about a non-recursive combineLatest: implementation? Is that naive?

No, it's not naive — in fact, that's how it was implemented originally — but it's a lot more work to maintain two completely separate methods that do the same thing.

However, if you have any ideas for simplifying the implementation of +combineLatest: while still calling through to +combineLatestWith:, we'd be happy to review a PR.

In the end, though, there will unfortunately be some cases for which RAC just isn't fast enough. We'd love to optimize wherever we can, but single-threaded imperative code will almost always be faster.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment