Defer creation of the TimeoutException when using the Single.timeout() operator #5250
Conversation
|
Please let me know if there is anything about this PR that needs to change in order to get this fix merged as the performance improvement seems to be reasonably significant. |
src/main/java/rx/Single.java
Outdated
| // since instantiating an exception will cause the current stack trace to be inspected | ||
| // and we only want to incur that overhead when a timeout actually happens. | ||
| other = Single.<T>defer(new Func0<Single<T>>() | ||
| { |
There was a problem hiding this comment.
This should go on the previous line after a space.
src/main/java/rx/Single.java
Outdated
| { | ||
| @Override | ||
| public Single<T> call() | ||
| { |
There was a problem hiding this comment.
This should go on the previous line after a space.
|
Is this also a problem without |
Codecov Report
@@ Coverage Diff @@
## 1.x #5250 +/- ##
============================================
+ Coverage 84.36% 84.38% +0.02%
- Complexity 2883 2884 +1
============================================
Files 290 290
Lines 18124 18125 +1
Branches 2479 2479
============================================
+ Hits 15290 15295 +5
+ Misses 1965 1964 -1
+ Partials 869 866 -3
Continue to review full report at Codecov.
|
|
@JakeWharton I havent done a super exhaustive audit but, it does not look like this is an issue with any of the other timeout operators in the 1.x branch or the single timeout in the 2.x branch |
| @@ -2254,7 +2254,15 @@ public final Completable toCompletable() { | |||
| */ | |||
| public final Single<T> timeout(long timeout, TimeUnit timeUnit, Single<? extends T> other, Scheduler scheduler) { | |||
| if (other == null) { | |||
| other = Single.<T> error(new TimeoutException()); | |||
| // Use a defer instead of simply other = Single.error(new TimeoutException()) | |||
There was a problem hiding this comment.
I'd inline this into SingleTimeout, that way, there are no extra instances unless necessary.
Use a defer instead of simply
other = Single.error(new TimeoutException())since instantiating an exception will cause the current stack trace to be inspected and that overhead should only be incurred when a timeout actually happens.I caught this problem as I was investigating unusually high CPU usage in one of the systems I operate. When measuring the impact of this change in a real world situation that makes only moderate use of this operator, I observed a 11% reduction in CPU usage.