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.

Sign up for GitHub

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

Jump to bottom

Add option to disable guarded delegate call #43

Merged
merged 4 commits into from
Aug 15, 2019
Merged

Add option to disable guarded delegate call #43

merged 4 commits into from
Aug 15, 2019

Conversation

ShaishavGandhi
Copy link
Collaborator

@ShaishavGandhi ShaishavGandhi commented Aug 14, 2019 •
edited
Loading

Resolves #20

Might have to wait for #41 to merge and i can rebase with the newer benchmarks.

Posting newer benchmarks:

Event throughput: grouped by number of events:

1 item (observable)
RxDogTagAndroidPerf.observable1_false                                                 423ns    0.000ms
RxDogTagAndroidPerf.observable1_true_withoutGuardedDelegate                        15,677ns    0.016ms  3606.15%
RxDogTagAndroidPerf.observable1_true                                               16,603ns    0.017ms  3825.06%

1 item (flowable)
RxDogTagAndroidPerf.flowable1_false                                                   320ns    0.000ms
RxDogTagAndroidPerf.flowable1_true_withoutGuardedDelegate                          16,157ns    0.016ms  4949.06%
RxDogTagAndroidPerf.flowable1_true                                                 16,765ns    0.017ms  5139.06%

1000 items (observable)
RxDogTagAndroidPerf.observable1000_false                                           27,913ns   0.028ms
RxDogTagAndroidPerf.observable1000_true_withoutGuardedDelegate                     49,496ns   0.049ms  77.32%
RxDogTagAndroidPerf.observable1000_true                                           249,479ns   0.249ms  793.77%

1000 items (flowable)
RxDogTagAndroidPerf.flowable1000_false                                             32,566ns   0.033ms
RxDogTagAndroidPerf.flowable1000_true_withoutGuardedDelegate                       51,765ns   0.052ms  58.95%
RxDogTagAndroidPerf.flowable1000_true                                             210,677ns   0.211ms  546.92%

1000000 items (observable)
RxDogTagAndroidPerf.observable1000000_false                                    26,984,534ns  26.985ms
RxDogTagAndroidPerf.observable1000000_true_withoutGuardedDelegate              34,621,670ns  34.622ms    28.30%
RxDogTagAndroidPerf.observable1000000_true                                    280,736,746ns  280.737ms   940.36%

1000000 items (flowable)
RxDogTagAndroidPerf.flowable1000000_false                                      29,811,097ns  29.811ms
RxDogTagAndroidPerf.flowable1000000_true_withoutGuardedDelegate                36,557,868ns  36.558ms    22.63%
RxDogTagAndroidPerf.flowable1000000_true                                      224,655,387ns  224.655ms   653.60%

Subscribe cost: grouped by complexity:

Simple (observable)
RxDogTagAndroidPerf.observable_false_subscribe_simple                                 148ns     0.000ms
RxDogTagAndroidPerf.observable_true_subscribe_simple_withoutGuardedDelegate        15,630ns     0.016ms  10460.81%
RxDogTagAndroidPerf.observable_true_subscribe_simple                               15,973ns     0.016ms  10692.57%

Simple (flowable)
RxDogTagAndroidPerf.flowable_false_subscribe_simple                                   198ns    0.000ms
RxDogTagAndroidPerf.flowable_true_subscribe_simple_withoutGuardedDelegate          15,881ns    0.016ms  7920.71%
RxDogTagAndroidPerf.flowable_true_subscribe_simple                                 15,895ns    0.016ms  7927.78%

Complex (observable)
RxDogTagAndroidPerf.observable_false_subscribe_complex                              5,331ns   0.005ms
RxDogTagAndroidPerf.observable_true_subscribe_complex                              22,095ns   0.022ms  314.46%
RxDogTagAndroidPerf.observable_true_subscribe_complex_withoutGuardedDelegate       23,033ns   0.023ms  332.06%

Complex (flowable)
RxDogTagAndroidPerf.flowable_true_subscribe_complex                                29,531ns   0.030ms
RxDogTagAndroidPerf.flowable_true_subscribe_complex_withoutGuardedCall             36,630ns   0.037ms  24.04%
RxDogTagAndroidPerf.flowable_false_subscribe_complex                               60,498ns   0.060ms  104.86%

E2E amortized cost:

Observable
RxDogTagAndroidPerf.observable_false_e2e                                          104,063ns  0.104ms
RxDogTagAndroidPerf.observable_true_e2e_withoutGuardedDelegate                    126,928ns  0.127ms  21.97%
RxDogTagAndroidPerf.observable_true_e2e                                           129,817ns  0.130ms  24.75%

Flowable
RxDogTagAndroidPerf.flowable_false_e2e                                            110,730ns  0.111ms
RxDogTagAndroidPerf.flowable_true_e2e_withoutGuardedDelegate                      152,865ns  0.153ms  38.05%
RxDogTagAndroidPerf.flowable_true_e2e                                             176,510ns  0.177ms  59.41%

ZacSweers
ZacSweers reviewed Aug 14, 2019
View reviewed changes
rxdogtag/src/main/java/com/uber/rxdogtag/RxDogTag.java Outdated
* during observer callbacks are intercepted and routed to RxDogTag's error handling that
* will give you more info on the subscription point. Set to true by default.
* @return this builder for fluent chaining.
* @see #guardedDelegateCall(NonCheckingConsumer, Runnable)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's not point to internal APIs

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point
f0bb0a3

rxdogtag/src/test/java/com/uber/anotherpackage/RxDogTagConfigurationTest.java Outdated
@@ -213,6 +213,26 @@ public void repackagingOENIEs_disabled() {
assertThat(cause.getStackTrace()[1].toString()).contains(RxDogTag.STACK_ELEMENT_SOURCE_HEADER);
}

@Test
public void guardObserverChecks_disabled() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you explain how this tests guarded delegate calls? I don't see a delegate implemented here that would throw

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It actually doesn't. It's just a sanity check to make sure that even if we disable guarded delegate calls, our normal workflow works correctly. It's basically checking the if-else statement in the DogTag*Observers

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's name or comment this to make that clear

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ae2e5d5

ZacSweers
ZacSweers approved these changes Aug 15, 2019
View reviewed changes
Copy link
Collaborator

@ZacSweers ZacSweers left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Go ahead and merge and I'll rebase #41 separately

@ShaishavGandhi ShaishavGandhi merged commit 43c433b into master Aug 15, 2019
@ShaishavGandhi ShaishavGandhi deleted the sg/guarded-delegate-config branch August 15, 2019 06:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ZacSweers ZacSweers ZacSweers approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Check performance of guarded delegate calls
2 participants
@ShaishavGandhi @ZacSweers