Save test duration moving average in Redis#56
Conversation
ebarajas
commented
Oct 30, 2025
ebarajas
commented
- Adds a new MovingAverage abstraction used to store test durations as a moving average in Redis
- This is calculated using the exponential moving average formula. This formula gives greater weight to the recent observations, which should help us better account for changes in test duration.
- Updates TestTimeRecord to also record duration using MovingAverage
- Updates the SuiteBinPacking to pull test durations from Redis when determining ordering, and fall back to the static timing file or a default duration if a time is not found.
ebarajas
requested review from
kahlstrom-figma,
peterzhu2118-figma,
ssteele110 and
yfang-figma
| LUA_SCRIPT= <<~LUA | ||
| local hash_key = KEYS[1] | ||
| local test_id = ARGV[1] | ||
| local new_duration = tonumber(ARGV[2]) | ||
| local smoothing = tonumber(ARGV[3]) | ||
| local current_avg = redis.call('HGET', hash_key, test_id) | ||
| if current_avg then | ||
| current_avg = tonumber(current_avg) | ||
| local new_avg = smoothing * new_duration + (1 - smoothing) * current_avg | ||
| redis.call('HSET', hash_key, test_id, new_avg) | ||
| return tostring(new_avg) | ||
| else | ||
| redis.call('HSET', hash_key, test_id, new_duration) | ||
| return tostring(new_duration) | ||
| end | ||
| LUA |
There was a problem hiding this comment.
Maybe put this into a separate file?
|
|
||
| def update(test_id, duration) | ||
| new_avg = @redis.eval(LUA_SCRIPT, keys: [@key], argv: [test_id, duration, @smoothing_factor]) | ||
| @values[test_id] = new_avg.to_f |
There was a problem hiding this comment.
I think this might not be useful given right now every time an update is called, a new MovingAverage instance is initialized and not reused?
Maybe we should just keep one instance of MovingAverage per build, or convert some fields to class variables?
There was a problem hiding this comment.
Agreed, I meant to only have one instance of MovingAverage but must have missed changing that
| load_all if @values.empty? | ||
| @values[test_id] |
There was a problem hiding this comment.
I think if @values has been updated somewhere else first just for a single value, then we won't call load_all and won't get the latest values.
I know this is not happening right now, because we kind of always call [] first (and update always creates a new instance of this class), but maybe we don't want to rely on that while we define these methods.
There was a problem hiding this comment.
Good point, I'll add a variable that tracks if it's has loaded in from redis or not yet
| # Represents a redis hash of moving averages for test durations | ||
| # | ||
| # Moving average is calculated using exponential moving average formula | ||
| class MovingAverage |
There was a problem hiding this comment.
Based on the fact that update is always called with a new instance of this class, does it mean that MovingAverage actually represents a "connection" to the redis hash instead of an in-memory cache of all the moving averages (so load_all is more like an internal optimization for consecutive reads)? Just want to make sure I understand the actual representation correctly.
ebarajas
force-pushed
the
ebarajas/save-moving-average
branch
from
a7b09c5 to
9842be0
Compare
4 participants
