Skip to content

Commit

Permalink
Improvement to the handling of every log statement to avoid creating …
Browse files Browse the repository at this point in the history
…unused log site stats. This was a big oversight because we were generating unused stats for every log statement when the vast majority didn't need it.

RELNOTES=Fix bug whereby unused LogSiteStats were being generated.
PiperOrigin-RevId: 510433440
  • Loading branch information
hagbard authored and Flogger Team committed Feb 17, 2023
1 parent cfbc8b3 commit bd2d607
Showing 1 changed file with 24 additions and 8 deletions.
32 changes: 24 additions & 8 deletions api/src/main/java/com/google/common/flogger/LogContext.java
Original file line number Diff line number Diff line change
Expand Up @@ -525,16 +525,22 @@ protected boolean postProcess(@NullableDecl LogSiteKey logSiteKey) {
// Without a log site we ignore any log-site specific behaviour.
if (logSiteKey != null) {
// Don't "return true" early from this code since we also need to handle stack size.
// Only get the stats instance when we need it, but cache it if we do. This avoids getting
// an instance for every log statement when the vast majority don't need it.
LogSiteStats stats = null;
Integer rateLimitCount = metadata.findValue(Key.LOG_EVERY_N);
RateLimitPeriod rateLimitPeriod = metadata.findValue(Key.LOG_AT_MOST_EVERY);
LogSiteStats stats = LogSiteStats.getStatsForKey(logSiteKey, metadata);
if (rateLimitCount != null && !stats.incrementAndCheckInvocationCount(rateLimitCount)) {
return false;
if (rateLimitCount != null) {
stats = ensureStats(stats, logSiteKey, metadata);
if (!stats.incrementAndCheckInvocationCount(rateLimitCount)) {
return false;
}
}

if (rateLimitPeriod != null
&& !stats.checkLastTimestamp(getTimestampNanos(), rateLimitPeriod)) {
return false;
RateLimitPeriod rateLimitPeriod = metadata.findValue(Key.LOG_AT_MOST_EVERY);
if (rateLimitPeriod != null) {
stats = ensureStats(stats, logSiteKey, metadata);
if (!stats.checkLastTimestamp(getTimestampNanos(), rateLimitPeriod)) {
return false;
}
}
}

Expand Down Expand Up @@ -568,6 +574,16 @@ protected boolean postProcess(@NullableDecl LogSiteKey logSiteKey) {
return true;
}

// Helper to get the stats instance on demand while reusing an instance if already obtained.
// Usage:
// // Possible null stats here...
// stats = ensureStats(stats, logSiteKey, metadata);
// // Non-null stats here (assigned to variable for next time)
private static LogSiteStats ensureStats(
@NullableDecl LogSiteStats stats, LogSiteKey logSiteKey, Metadata metadata) {
return stats != null ? stats : LogSiteStats.getStatsForKey(logSiteKey, metadata);
}

/**
* Pre-processes log metadata and determines whether we should make the pending logging call.
*
Expand Down

0 comments on commit bd2d607

Please sign in to comment.