logd: remove min heap in SerializedFlushToState
There was a bug in SerializedFlushToState::Prune() caused by an access
to a SerializedLogEntry raw pointer as a member of a MinHeapElement,
which was deleted earlier in the function.
Instead of just fixing the order of the access and the deletion, I
sought out to remove the raw pointer entirely. In doing so, I noticed
that the min heap doesn't provide significant benefit, since we'll
only ever have 8 log buffers so scalability is not an issue.
Therefore this change removes the min heap entirely and uses the
existing log_position_ and logs_needed_from_next_position_ members to
keep track of which are the next unread logs.
It also adds a smoke test for SerializedFlushToState::Prune() and
additional CHECK() statements to help prevent future errors.
Bug: 168869299
Test: unit tests
Change-Id: Id4d5fdbaff2fe6dc49c38f01e73f900f84d3696b
diff --git a/logd/SerializedFlushToState.cpp b/logd/SerializedFlushToState.cpp
index b02ccc3..378cf20 100644
--- a/logd/SerializedFlushToState.cpp
+++ b/logd/SerializedFlushToState.cpp
@@ -16,6 +16,8 @@
#include "SerializedFlushToState.h"
+#include <limits>
+
#include <android-base/logging.h>
SerializedFlushToState::SerializedFlushToState(uint64_t start, LogMask log_mask)
@@ -63,14 +65,13 @@
log_positions_[log_id].emplace(log_position);
}
-void SerializedFlushToState::AddMinHeapEntry(log_id_t log_id) {
+void SerializedFlushToState::UpdateLogsNeeded(log_id_t log_id) {
auto& buffer_it = log_positions_[log_id]->buffer_it;
auto read_offset = log_positions_[log_id]->read_offset;
- // If there is another log to read in this buffer, add it to the min heap.
+ // If there is another log to read in this buffer, let it be read.
if (read_offset < buffer_it->write_offset()) {
- auto* entry = buffer_it->log_entry(read_offset);
- min_heap_.emplace(log_id, entry);
+ logs_needed_from_next_position_[log_id] = false;
} else if (read_offset == buffer_it->write_offset()) {
// If there are no more logs to read in this buffer and it's the last buffer, then
// set logs_needed_from_next_position_ to wait until more logs get logged.
@@ -85,8 +86,7 @@
if (buffer_it->write_offset() == 0) {
logs_needed_from_next_position_[log_id] = true;
} else {
- auto* entry = buffer_it->log_entry(0);
- min_heap_.emplace(log_id, entry);
+ logs_needed_from_next_position_[log_id] = false;
}
}
} else {
@@ -106,24 +106,41 @@
}
CreateLogPosition(i);
}
- logs_needed_from_next_position_[i] = false;
- // If it wasn't possible to insert, logs_needed_from_next_position will be set back to true.
- AddMinHeapEntry(i);
+ UpdateLogsNeeded(i);
}
}
-MinHeapElement SerializedFlushToState::PopNextUnreadLog() {
- auto top = min_heap_.top();
- min_heap_.pop();
+bool SerializedFlushToState::HasUnreadLogs() {
+ CheckForNewLogs();
+ log_id_for_each(i) {
+ if (log_positions_[i] && !logs_needed_from_next_position_[i]) {
+ return true;
+ }
+ }
+ return false;
+}
- auto* entry = top.entry;
- auto log_id = top.log_id;
+LogWithId SerializedFlushToState::PopNextUnreadLog() {
+ uint64_t min_sequence = std::numeric_limits<uint64_t>::max();
+ log_id_t log_id;
+ const SerializedLogEntry* entry = nullptr;
+ log_id_for_each(i) {
+ if (!log_positions_[i] || logs_needed_from_next_position_[i]) {
+ continue;
+ }
+ if (log_positions_[i]->log_entry()->sequence() < min_sequence) {
+ log_id = i;
+ entry = log_positions_[i]->log_entry();
+ min_sequence = entry->sequence();
+ }
+ }
+ CHECK_NE(nullptr, entry);
log_positions_[log_id]->read_offset += entry->total_len();
logs_needed_from_next_position_[log_id] = true;
- return top;
+ return {log_id, entry};
}
void SerializedFlushToState::Prune(log_id_t log_id,
@@ -133,25 +150,12 @@
return;
}
- // // Decrease the ref count since we're deleting our reference.
+ // Decrease the ref count since we're deleting our reference.
buffer_it->DecReaderRefCount();
// Delete in the reference.
log_positions_[log_id].reset();
- // Remove the MinHeapElement referencing log_id, if it exists, but retain the others.
- std::vector<MinHeapElement> old_elements;
- while (!min_heap_.empty()) {
- auto& element = min_heap_.top();
- if (element.log_id != log_id) {
- old_elements.emplace_back(element);
- }
- min_heap_.pop();
- }
- for (auto&& element : old_elements) {
- min_heap_.emplace(element);
- }
-
// Finally set logs_needed_from_next_position_, so CheckForNewLogs() will re-create the
// log_position_ object during the next read.
logs_needed_from_next_position_[log_id] = true;