freenet
Cleanup RunningAverage implementations
the RunningAverage implementations have various issues:
- Too many responsibilities (like TimeDecayingRunningAverage, that tries to keep track of clock skew in the middle of its calculations)
- Too much mutable state (like SimpleRunningAverage, that keeps track of "how many items do I have" across 3 different fields)
- A generous amount of synchronization, but not everywhere
- Leftover methods from earlier times (custom serialization formats and unused code that does who-knows-what)
- A large amount of empty Javadoc comment
- A general lack of consistency in code formatting
- Zero tests
This PR attempts to improve the situation by:
- Separating out the clock skew tracking & reporting out of the main logic of TimeDecayingRunningAverage (and using a monotonic clock for the actual computations)
- Keeping the state in a single immutable object accessed through an AtomicReference where possible, so state is in one place and can easily be reasoned about
- Replacing synchronization with compare-and-set operations on the reference
- Removing the legacy format read and write method, and removing Serializable from the class hierarchy (this is technically a breaking, but in my view it is highly unlikely that anybody would be using these methods)
- Removing the empty Javadoc. Most of the operations are explained on the RunningAverage interface; others are sufficiently clear.
- Reformat everything (consistent use of tabs vs spaces, yay!), move constructors to the top, etc.
- Add some tests to cover most of the functionality
Also, deprecate MedianMeanRunningAverage as it's probably never a good idea to use it.
Thank you for this PR! It already looks really neat. And getting rid of synchronization in so many places should make our huge-amount-of-threads setup quite a bit more efficient.
And I’m sorry that it took so long to review. I shied away from the size of the diff, but in hindsight this was nicely readable and easy to understand. :+1:
@ArneBab thank you for your review!
Addressed your comment regarding the additional assertion and rebased this branch on the latest next.
Hope I cleared up the TrivialRunningAverage discussions too - just let me know if we're on the same page now :smile: