logd: address code fragility in last watermarks
Do not make the assumption that if worstPid is set, that the log
buffer id is not LOG_ID_EVENTS or LOG_ID_SECURITY. Add comments
to prevent future over-optimization based on this assumption.
Make sure we reset mLast[id] = begin() when we mark it unset, but
tell optimizer this is an _impossible_ path.
SideEffects: drop two branches in all erase calls, gain an unordered
find() on an empty list for events and security buffers.
Test: gTest logd-unit-tests, liblog-unit-test & logcat-unit-tests
Bug: 32247044
Change-Id: Ic156ca2253c050c28021cedf48bedaf7bd692c09
diff --git a/logd/LogBuffer.cpp b/logd/LogBuffer.cpp
index aff8a46..5cab7a8 100644
--- a/logd/LogBuffer.cpp
+++ b/logd/LogBuffer.cpp
@@ -18,6 +18,7 @@
#include <errno.h>
#include <stdio.h>
#include <string.h>
+#include <sys/cdefs.h>
#include <sys/user.h>
#include <time.h>
#include <unistd.h>
@@ -31,6 +32,10 @@
#include "LogKlog.h"
#include "LogReader.h"
+#ifndef __predict_false
+#define __predict_false(exp) __builtin_expect((exp) != 0, 0)
+#endif
+
// Default
#define log_buffer_size(id) mMaxSize[id]
@@ -234,9 +239,10 @@
}
}
- if ((id != LOG_ID_EVENTS) && (id != LOG_ID_SECURITY)) {
+ { // start of scope for pid found iterator
// element->getUid() may not be AID_SYSTEM for next-best-watermark.
- // start of scope for pid found iterator
+ // will not assume id != LOG_ID_EVENTS or LOG_ID_SECURITY for KISS and
+ // long term code stability, find() check should be fast for those ids.
LogBufferPidIteratorMap::iterator found =
mLastWorstPidOfSystem[id].find(element->getPid());
if ((found != mLastWorstPidOfSystem[id].end())
@@ -254,10 +260,11 @@
if (doSetLast) {
log_id_for_each(i) {
if (setLast[i]) {
- if (it == mLogElements.end()) { // unlikely
+ if (__predict_false(it == mLogElements.end())) { // impossible
mLastSet[i] = false;
+ mLast[i] = mLogElements.begin();
} else {
- mLast[i] = it;
+ mLast[i] = it; // push down the road as next-best-watermark
}
}
}
@@ -420,7 +427,7 @@
LogBufferElementCollection::iterator it;
- if (caller_uid != AID_ROOT) {
+ if (__predict_false(caller_uid != AID_ROOT)) { // unlikely
// Only here if clear all request from non system source, so chatty
// filter logistics is not required.
it = mLastSet[id] ? mLast[id] : mLogElements.begin();
@@ -472,6 +479,7 @@
if ((id == LOG_ID_EVENTS) || (id == LOG_ID_SECURITY)) {
stats.sortTags(AID_ROOT, (pid_t)0, 2, id).findWorst(
worst, worst_sizes, second_worst_sizes, threshold);
+ // per-pid filter for AID_SYSTEM sources is too complex
} else {
stats.sort(AID_ROOT, (pid_t)0, 2, id).findWorst(
worst, worst_sizes, second_worst_sizes, threshold);
@@ -505,8 +513,9 @@
it = found->second;
}
}
- if (worstPid) { // Only set if !LOG_ID_EVENTS and !LOG_ID_SECURITY
- // begin scope for pid worst found iterator
+ if (worstPid) { // begin scope for pid worst found iterator
+ // FYI: worstPid only set if !LOG_ID_EVENTS and
+ // !LOG_ID_SECURITY, not going to make that assumption ...
LogBufferPidIteratorMap::iterator found
= mLastWorstPidOfSystem[id].find(worstPid);
if ((found != mLastWorstPidOfSystem[id].end())
@@ -596,7 +605,8 @@
|| (mLastWorstPidOfSystem[id].find(element->getPid())
== mLastWorstPidOfSystem[id].end()))) {
// element->getUid() may not be AID_SYSTEM, next best
- // watermark if current one empty.
+ // watermark if current one empty. id is not LOG_ID_EVENTS
+ // or LOG_ID_SECURITY because of worstPid check.
mLastWorstPidOfSystem[id][element->getPid()] = it;
}
if ((!gc && !worstPid && (key == worst))
@@ -640,7 +650,8 @@
|| (mLastWorstPidOfSystem[id].find(worstPid)
== mLastWorstPidOfSystem[id].end()))) {
// element->getUid() may not be AID_SYSTEM, next best
- // watermark if current one empty.
+ // watermark if current one empty. id is not
+ // LOG_ID_EVENTS or LOG_ID_SECURITY because of worstPid.
mLastWorstPidOfSystem[id][worstPid] = it;
}
if ((!gc && !worstPid) ||