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

GEODE-10010: Refine per-second Redis stats #7437

Merged
merged 2 commits into from Mar 15, 2022

Conversation

DonalEvans
Copy link
Contributor

@DonalEvans DonalEvans commented Mar 9, 2022

Code changes:

  • Rather than a value that updates once per second, instantaneous
    operations per second and instantaneous kilobytes read per second now
    return a rolling average of those stats updated 16 times per second
  • Change instantaneous operations per second type from double to int,
    since it's always a whole number
  • Introduce RollingAverageStat nested class in RedisStats to handle
    calculating the rolling average value of a given stat

Test changes:

  • Rename AbstractRedisInfoStatsIntegrationTest to match child classes
  • Rather than measuring instantaneous per second stats after
    operations have finished, sample them while operations are ongoing
  • Assert only that the stats have updated, not that they are close to a
    calculated expected value, as this proved too inconsistent and very
    flaky

Authored-by: Donal Evans doevans@vmware.com

For all changes:

  • Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?

  • Has your PR been rebased against the latest commit within the target branch (typically develop)?

  • Is your initial contribution a single, squashed commit?

  • Does gradlew build run cleanly?

  • Have you written or updated unit tests to verify your changes?

  • If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under ASF 2.0?

@DonalEvans DonalEvans added redis Issues related to the geode-for-redis module windows add this label to get Windows JDK11 PR checks too labels Mar 9, 2022
Code changes:
 - Rather than a value that updates once per second, instantaneous
 operations per second and instantaneous kilobytes read per second now
 return a rolling average of those stats updated 16 times per second
 - Change instantaneous operations per second type from double to int,
 since it's always a whole number
 - Introduce RollingAverageStat nested class in RedisStats to handle
 calculating the rolling average value of a given stat

Test changes:
 - Rename AbstractRedisInfoStatsIntegrationTest to match child classes
 - Rather than retrieving instantaneous per second stats after
 operations have finished, sample them while operations are ongoing
 - Assert only that the stats have updated, not that they are close to a
 calculated expected value, as this proved too inconsistent and very
 flaky
 - Use default duration for await() in connected clients stat test to
 prevent flakiness

Authored-by: Donal Evans <doevans@vmware.com>
Comment on lines 237 to 252
private long calculate() {
long currentValue;
try {
currentValue = statCallable.call();
} catch (Exception e) {
throw new RedisException("Error while calculating RollingAverage stats", e);
}
long delta = currentValue - valueReadLastTick;
valueReadLastTick = currentValue;
valuesReadOverLastNSamples[tickNumber] = delta;
tickNumber++;
if (tickNumber >= valuesReadOverLastNSamples.length) {
tickNumber = 0;
}
return Arrays.stream(valuesReadOverLastNSamples).sum();
}
Copy link
Contributor

Choose a reason for hiding this comment

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

For fun I did some quick benchmarking and came up with this optimization:

    long currentTotal = 0;
    public long calculate2() {
      long currentValue;
      try {
        currentValue = statCallable.call();
      } catch (Exception e) {
        throw new RedisException("Error while calculating RollingAverage stats", e);
      }
      long delta = currentValue - valueReadLastTick;
      valueReadLastTick = currentValue;

      currentTotal = currentTotal + delta - valuesReadOverLastNSamples[tickNumber];
      valuesReadOverLastNSamples[tickNumber] = delta;
      // same as mod (%) but measurably faster...
      tickNumber = (tickNumber + 1) & (valuesReadOverLastNSamples.length - 1);

      return currentTotal;
    }

This runs more than an order of magnitude faster. Granted, the benchmark was hammering this as fast as possible so at only 16 times / second the optimization would probably be negligible in the real world. Your call if you want to do it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Faster is better!

Authored-by: Donal Evans <doevans@vmware.com>
@DonalEvans DonalEvans merged commit be028f2 into apache:develop Mar 15, 2022
@DonalEvans DonalEvans deleted the GEODE-10010-opsPerSecondStat branch March 15, 2022 15:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
redis Issues related to the geode-for-redis module windows add this label to get Windows JDK11 PR checks too
Projects
None yet
2 participants