Track our time buffer more explicitly
We were getting crashes because we couldn't register the timer inside
LogReader because we had forgotten how to translate the current time
(t=0). That's wrong...
The root cause was that our concept of how much to buffer was too
loose. We really needed to be pruning off the current time, not off the
size of the buffer, since there is nothing saying that we haven't
buffered too far into the future and forget now. Add a callback from
the scheduler for this.
Change-Id: I4b396b4e317140ff2abfe7b573bf7c3dae157e5c
Signed-off-by: Austin Schuh <austin.schuh@bluerivertech.com>
diff --git a/aos/events/event_scheduler.cc b/aos/events/event_scheduler.cc
index 49b170d..3739f49 100644
--- a/aos/events/event_scheduler.cc
+++ b/aos/events/event_scheduler.cc
@@ -50,6 +50,8 @@
::std::function<void()> callback = ::std::move(iter->second);
events_list_.erase(iter);
callback();
+
+ converter_->ObserveTimePassed(scheduler_scheduler_->distributed_now());
}
void EventScheduler::RunOnRun() {
diff --git a/aos/events/event_scheduler.h b/aos/events/event_scheduler.h
index 397b5f0..f981ef2 100644
--- a/aos/events/event_scheduler.h
+++ b/aos/events/event_scheduler.h
@@ -58,6 +58,9 @@
// node.
virtual monotonic_clock::time_point FromDistributedClock(
size_t node_index, distributed_clock::time_point time) = 0;
+
+ // Called whenever time passes this point and we can forget about it.
+ virtual void ObserveTimePassed(distributed_clock::time_point time) = 0;
};
class EventSchedulerScheduler;
@@ -151,6 +154,8 @@
size_t /*node_index*/, distributed_clock::time_point time) override {
return monotonic_clock::epoch() + time.time_since_epoch();
}
+
+ void ObserveTimePassed(distributed_clock::time_point /*time*/) override {}
};
UnityConverter unity_converter_;
diff --git a/aos/events/event_scheduler_test.cc b/aos/events/event_scheduler_test.cc
index 1ad0a84..3bcf6ca 100644
--- a/aos/events/event_scheduler_test.cc
+++ b/aos/events/event_scheduler_test.cc
@@ -41,6 +41,8 @@
distributed_offset_[node_index];
}
+ void ObserveTimePassed(distributed_clock::time_point /*time*/) override {}
+
private:
// Offset to the distributed clock.
// distributed = monotonic + offset;
diff --git a/aos/network/multinode_timestamp_filter.cc b/aos/network/multinode_timestamp_filter.cc
index a43fdfd..ebb7a19 100644
--- a/aos/network/multinode_timestamp_filter.cc
+++ b/aos/network/multinode_timestamp_filter.cc
@@ -353,11 +353,26 @@
CHECK(!times_.empty())
<< ": Found no times to do timestamp estimation, please investigate.";
+}
+
+void InterpolatedTimeConverter::ObserveTimePassed(
+ distributed_clock::time_point time) {
// Keep at least 500 points and time_estimation_buffer_seconds seconds of
// time. This should be enough to handle any reasonable amount of history.
- while (times_.size() > kHistoryMinCount &&
- std::get<0>(times_.front()) + time_estimation_buffer_seconds_ <
- std::get<0>(times_.back())) {
+ while (true) {
+ if (times_.size() < kHistoryMinCount) {
+ return;
+ }
+ if (std::get<0>(times_[1]) + time_estimation_buffer_seconds_ > time) {
+ VLOG(1) << "Not popping because "
+ << std::get<0>(times_[1]) + time_estimation_buffer_seconds_
+ << " > " << time;
+ return;
+ }
+
+ VLOG(1) << "Popping sample because " << times_.size() << " > "
+ << kHistoryMinCount << " && " << std::get<0>(times_[1]) << " < "
+ << time - time_estimation_buffer_seconds_;
times_.pop_front();
have_popped_ = true;
}
diff --git a/aos/network/multinode_timestamp_filter.h b/aos/network/multinode_timestamp_filter.h
index 929c837..96f7228 100644
--- a/aos/network/multinode_timestamp_filter.h
+++ b/aos/network/multinode_timestamp_filter.h
@@ -177,6 +177,9 @@
monotonic_clock::time_point FromDistributedClock(
size_t node_index, distributed_clock::time_point time) override;
+ // Called whenever time passes this point and we can forget about it.
+ void ObserveTimePassed(distributed_clock::time_point time) override;
+
private:
// Returns the next timestamp, or nullopt if there isn't one. It is assumed
// that if there isn't one, there never will be one.
diff --git a/aos/network/multinode_timestamp_filter_test.cc b/aos/network/multinode_timestamp_filter_test.cc
index f1cf0aa..bdc1992 100644
--- a/aos/network/multinode_timestamp_filter_test.cc
+++ b/aos/network/multinode_timestamp_filter_test.cc
@@ -223,13 +223,8 @@
EXPECT_EQ(me + chrono::milliseconds(10),
time_converter.FromDistributedClock(0, de + kDt));
- // Force 10.1 seconds now. This will forget the 0th point at the origin.
- EXPECT_EQ(
- de + kDefaultHistoryDuration + kDt,
- time_converter.ToDistributedClock(0, me + kDefaultHistoryDuration + kDt));
- EXPECT_EQ(me + kDefaultHistoryDuration + kDt,
- time_converter.FromDistributedClock(
- 0, de + kDefaultHistoryDuration + kDt));
+ // Now force ourselves to forget.
+ time_converter.ObserveTimePassed(de + kDefaultHistoryDuration + kDt * 3 / 2);
// Yup, can't read the origin anymore.
EXPECT_DEATH({ LOG(INFO) << time_converter.ToDistributedClock(0, me); },