Scheduled action no interrupt

[akarnokd]

At the end of the run, ScheduledAction unsubscribes itself which triggers a set of cleanup actions, and since it is also the way to cancel a running action, it calls future.cancel(true) as well which interrupts the task in the underlying executor framework. However, on a normal completion path, it is unnecessary to interrupt itself at the end of the run since it will complete anyway, and not to mention, it has some performance implications for short running tasks. The downside is the +4 bytes per ScheduledAction.

[akarnokd]

Some perf (* this PR)

Benchmark                                   (size)   Mode   Samples        Score  Score error    Units
r.s.ComputationSchedulerPerf.observeOn           1  thrpt         5   130147,392     6501,097    ops/s
r.s.ComputationSchedulerPerf.observeOn*          1  thrpt         5   131074,109     1993,794    ops/s
r.s.ComputationSchedulerPerf.observeOn        1000  thrpt         5     5358,665      593,569    ops/s
r.s.ComputationSchedulerPerf.observeOn*       1000  thrpt         5     3251,381       48,089    ops/s
r.s.ComputationSchedulerPerf.observeOn     1000000  thrpt         5        9,189        0,162    ops/s
r.s.ComputationSchedulerPerf.observeOn*    1000000  thrpt         5       10,913        0,387    ops/s
r.s.ComputationSchedulerPerf.subscribeOn         1  thrpt         5   143923,587     1458,234    ops/s
r.s.ComputationSchedulerPerf.subscribeOn*        1  thrpt         5   152830,004     2317,941    ops/s
r.s.ComputationSchedulerPerf.subscribeOn      1000  thrpt         5    26168,282    18301,891    ops/s
r.s.ComputationSchedulerPerf.subscribeOn*     1000  thrpt         5    29264,460     1546,900    ops/s
r.s.ComputationSchedulerPerf.subscribeOn   1000000  thrpt         5       35,739        2,368    ops/s
r.s.ComputationSchedulerPerf.subscribeOn*  1000000  thrpt         5       36,643        1,678    ops/s

I've rerun observeOn * 1000 a couple of times and I always get such low values which I don't understand; could be the effect of the increased memory usage tripping over some cache sizes.

[akarnokd]

Somehow, observeOn x 1000 runs slower no matter what optimizations I try, but for the rest, the changes are generally improving the throughput.

[benjchristensen]

This isn't Atomic like the ONCE_UPDATER was, so are you just trusting the cancel Subscription to be idempotent in the event of a race?

[akarnokd]

Yes. I ran some profiling and saw cancel.unsubscribe() to be expensive. Adding this check reduced the time spent in unsubscribe() by half. Usually, there isn't any race but just the self cancellation and then when the ScheduledAction is removed from its parent's CompositeSubscription.

[benjchristensen]

I don't understand. The ONCE_UPDATER would be sure we only call unsubscribe once, and it needs to be called at least once, so what performance benefit happened by removing ONCE_UPDATER that now allows it to possibly be called more than once?

[akarnokd]

It is known the ScheduledAction.unsubscribe is called at least twice: once at the end of the run and a second time because the task is removed via CompositeSubscription.remove() from the parent scheduler. If the task terminates normally, the second call to unsubscribe will guaranteed to fail the CAS which has high cost anyways. Testing the cancel.isUnsubscribed() is effectively a volatile read with far less cost. In addition, it saved 4 bytes of memory per instance now usable by passing the thread around.

[benjchristensen]

What is the cost of a failed CAS compared with a volatile read?

I'm not arguing that testing isUnsubscribed() is cheap or expensive, just that it's not atomic. We could end up calling unsubscribe twice without the CAS check.

[akarnokd]

On X86, volatile read cost the same as any regular memory read. CAS is around 20-40 cycles. I've run a small program and measured always failing and always succeding CAS on my i7 4770 @ 3.5GHz and gave me ~5.2 ns in both cases, If I test the variable before CAS, I get 1.6ns for always fails and 8.8ns for always succeeding. So for 2 unsubscribe calls, we get 10.4ns in the CAS only case and 10.4ns with read-before-cas. However, any additional unsubscribe calls adds only 1.6ns with read-before-cas instead of 5.2ns. I must admit, the 2 unsubscribe calls costing the same time suprises me.

The unsubscribe() is still idempotent because either cancel.isUnsubscribed returns true and nothing happens or cancel.unsubscribe() is called which is idempotent.

public class CasFailureTest {
    public static void main(String[] args) {
        AtomicInteger v = new AtomicInteger(0);
//      AtomicInteger v = new AtomicInteger(1);

        int k = 100_000_000;
        for (int i = 0; i < 10; i++) {
            long n = System.nanoTime();
            for (int j = 0; j < k; j++) {
                if (v.get() == j) {
                    v.compareAndSet(j, j + 1);
                }
//              if (v.get() == 0)
//                  v.compareAndSet(0, 1);
            }
            System.out.println(1d * (System.nanoTime() - n) / k);
            v.set(0);
        }
    }
}

[benjchristensen]

That is great information! Do you think it would be valuable to convert this into a JMH test and commit it with the baseline benchmarks similar to https://github.com/ReactiveX/RxJava/blob/1.x/src/perf/java/rx/PerfBaseline.java along with a README of what costs are for these different things?

[benjchristensen]

or cancel.unsubscribe() is called which is idempotent.

This is the part that I wasn't sure of, so if that is idempotent then there definitely is not a need for the extra ONCE_UPDATER. Thanks for walking me through this patiently.

[akarnokd]

As an exercise to learn JMH, sure. Otherwise, it would be a curiosity only.

[benjchristensen]

Merging as these changes look good and I understand them now well enough to feel confident with them.

The discussion about whether we should be interrupting by default is somewhat orthogonal to this PR. This makes the current behavior work more safely.

Thank you @akarnokd

[akarnokd]

See #1911 for the JMH benchmarks.

[daschl]

@benjchristensen @akarnokd with 1.0.2 my interrupts are gone!