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.

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

Cleanup RunningAverage implementations #989

Open
wants to merge 4 commits into
base: next
Choose a base branch
from

Conversation

bertm
Copy link
Contributor

@bertm bertm commented Oct 13, 2024

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.

Simplify the implementations of the RunningAverage interface by
separating the data being kept into an immutable object. This way we no
longer have to synchronize on everything, but rather update the data
object using compare-and-set.

MedianMeanRunningAverage is not included in this effort because that
performance would be hurt due to the large amount of data kept.
The variables nextSlotPtr and curLen can trivially be derived from
totalReports, remove the former variables.

Additionally, synchronize on the internal array rather than on the
instance itself to prevent potential locking issues.
This fixes lack of synchronization in the copy constructor, removes
all the empty javadoc and generally improves code style.
Replace all uses of MedianMeanRunningAverage with TrivialRunningAverage
which does not eat an unbounded amount of RAM. This class was only used
for debug purposes under the logMINOR flag. Having the median value in
addition to mean value does likely not warrant the additional RAM and
CPU consumption.

Deprecate the class so that it can eventually be removed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant