Fix #125 Refreshable Performance

[carterkozak]

Before this PR

coarse locking, java8, no cleaners
Before benchmarks:

Benchmark                     Mode  Cnt     Score       Error  Units
RefreshableBenchmark.map      avgt    4   225.953 ±  1002.288  us/op
RefreshableBenchmark.slowMap  avgt    4  5290.169 ± 62649.044  ms/op

After this PR

Minimum java 11 (for the new Cleaner API), Read/Write locks
After benchmarks (from #129):

Benchmark                     Mode  Cnt  Score    Error  Units
RefreshableBenchmark.map      avgt    4  6.648 ± 15.174  us/op
RefreshableBenchmark.slowMap  avgt    4  2.656 ±  0.088  ms/op

Read/Write locking

Readers do not need an exclusive lock, this results in a significant improvement in the slowMap benchmark results over 1.2.0.
Writers are given exclusive access and may result in jitter, but refreshable updates tend to be relatively few and far between unless there are object equality problems.

Cleaner

The Cleaner uses a background thread to dispose of objects when they're no longer referenced. This allows us to move work off threads which request updates in favor of the single background thread. Note that it's possible many threads may concurrently map or subscribe while only one is cleaning up references, so in an exceedingly hot loop this can result in memory pressure. However, in the original implementation we were limited to a single thread at any given time, it was just borrowed from a caller, and worked under an exclusive lock to prevent other threads from mapping in parallel.

==COMMIT_MSG==
Refreshable Performance improvements to prevent services from grinding to a halt when they use Refreshable in unintended ways. This change requires a java 11 minimum version.
==COMMIT_MSG==

Possible downsides?

Oh goodness yes! Instead of blocking on locks when map functions are provided we can spin along quickly and happily until we OOM because there's a single cleaner thread which cannot clean as fast as N other threads call map. This scenario was already broken, and it's not likely we're in loops that hot, so I would consider this a reasonable trade-off.

Concurrency is hard, this likely introduces a bug or two that we'll have to discover and fix. It dulls some sharp edges so that some types of misuse are less problematic, so the trade-off is likely reasonable.

Bumps to java 11

Alternatives

We could produce 'lazy' refreshable instances from the map function which don't require subscriptions on their parents until subscribe is called on the instance or one of its children. The vast majority of refreshables are never subscribed but only used as convenient fluent suppliers. This approach would present some serious risks: today each current or get call is guaranteed to be immediate, at worst a volatile read, however a lazy implementation would execute the mapping function on the calling thread, potentially blocking other callers, or impacting subscriber update order. I'm not comfortable with that sort of change at the moment, but I suppose I could be convinced otherwise -- I think the risks and additional complexity outweigh the upside compared to the optimization in this PR.

[changelog-app]

Generate changelog in changelog/@unreleased

Type

• Feature
• Improvement
• Fix
• Break
• Deprecation
• Manual task
• Migration

Description

Refreshable Performance improvements to prevent services from grinding to a halt when they use Refreshable in unintended ways. This change is considered a break because it requires java 11.

Check the box to generate changelog(s)

• Generate changelog entry

[jkozlowski]

Just some comments. I do wonder what are you aiming for here, what's acceptable in terms of performance?

[carterkozak]

I do wonder what are you aiming for here, what's acceptable in terms of performance?

hoping to resolve the problem represented by slowMap benchmark, where slow code in hot paths becomes exceedingly slow. This change reduces avg time from 5 seconds to 2.5 milliseconds. Raw map calls are much more consistent as well.

[carterkozak]

Ah I never linked this PR to the #125 issue! That issue has some context based on a few internal P0s.

[jkozlowski]

Releasing comments so far

[jkozlowski]

I have spent way too much time on this, but I like reviewing and understanding code like this. My suggestions are on this PR #144, I don't think any of them are blocking (except for renames where you changed responsibilities of classes), and I haven't been able to find fault in any of the code here and I spent about 1.5h 🕐 just grokking all the different responsibilities and flows this class has.

[jkozlowski]

For documentation purposes and error prevention, would you consider doing:

private Disposable subscribeToSelf(SideEffectSubscriber<? super T> subscriber) {
  return subscribeToSelfImpl(subscriber);
}

private Disposable subscribeToSelf(SelfRemovingMapSubscriber subscriber) {
  return subscribeToSelf(subscriber);
}

private Disposable subscribeToSelfImpl(Consumer<? super T> subscriber) {
  ...
}

We never want unsafe subscriber to be passed in here.

[jkozlowski]

This class name is no longer accurate: it is not responsible for removing itself at all!

[jkozlowski]

Would you consider refactoring a tiny method to make the it abundantly clear that we abide by the cleaner contract for register which states that the clearning Runnable should never reference the object it is supposed to clean up after? (code written in browser so might not be 100% correct, but I hope you get what I mean).

private Disposable subscribeMapper(WeakReference<SettableRefreshable<R>> child, Function<? super T, R> function) {
  SelfRemovingMapSubscriber<? super T, R> mapSubscriber = new SelfRemovingMapSubscriber<>(function, child);
return subscribeToSelf(mapSubscriber);
}

[svc-autorelease]

Refusing to release a major rev - please do this manually at https://autorelease.bots/palantir/refreshable