Skip to content
Permalink

Comparing changes

Choose two branches to see what’s changed or to start a new pull request. If you need to, you can also or learn more about diff comparisons.

Open a pull request

Create a new pull request by comparing changes across two branches. If you need to, you can also . Learn more about diff comparisons here.
base repository: mockingbirdnest/Principia
Failed to load repositories. Confirm that selected base ref is valid, then try again.
Loading
base: b42a3fdee65b
Choose a base ref
...
head repository: mockingbirdnest/Principia
Failed to load repositories. Confirm that selected head ref is valid, then try again.
Loading
compare: be551e1d21be
Choose a head ref
  • 5 commits
  • 5 files changed
  • 1 contributor

Commits on May 12, 2019

  1. Copy the full SHA
    53c1f3f View commit details
  2. Fix the test.

    pleroy committed May 12, 2019
    Copy the full SHA
    4559236 View commit details
  3. Another test fix.

    pleroy committed May 12, 2019
    Copy the full SHA
    c88f51d View commit details
  4. After egg's review.

    pleroy committed May 12, 2019
    Copy the full SHA
    36baf3f View commit details
  5. Merge pull request #2168 from pleroy/2165

    Guard the ephemeris as soon as we construct the prognosticator parameters
    pleroy authored May 12, 2019
    Copy the full SHA
    be551e1 View commit details
Showing with 83 additions and 42 deletions.
  1. +43 −33 ksp_plugin/vessel.cpp
  2. +6 −2 ksp_plugin/vessel.hpp
  3. +4 −2 ksp_plugin_test/vessel_test.cpp
  4. +4 −4 physics/ephemeris.hpp
  5. +26 −1 physics/ephemeris_body.hpp
76 changes: 43 additions & 33 deletions ksp_plugin/vessel.cpp
Original file line number Diff line number Diff line change
@@ -72,8 +72,12 @@ Vessel::~Vessel() {
if (prognosticator_.joinable()) {
{
absl::MutexLock l(&prognosticator_lock_);
CHECK(prognosticator_parameters_);
prognosticator_parameters_->shutdown = true;
prognosticator_parameters_ =
PrognosticatorParameters{Ephemeris<Barycentric>::Guard(ephemeris_),
psychohistory_->last().time(),
psychohistory_->last().degrees_of_freedom(),
prediction_adaptive_step_parameters_,
/*shutdown=*/true};
}
prognosticator_.join();
}
@@ -206,8 +210,10 @@ void Vessel::set_prediction_adaptive_step_parameters(
prediction_adaptive_step_parameters) {
prediction_adaptive_step_parameters_ = prediction_adaptive_step_parameters;
absl::MutexLock l(&prognosticator_lock_);
prognosticator_parameters_->adaptive_step_parameters =
prediction_adaptive_step_parameters;
if (prognosticator_parameters_) {
prognosticator_parameters_->adaptive_step_parameters =
prediction_adaptive_step_parameters;
}
}

Ephemeris<Barycentric>::AdaptiveStepParameters const&
@@ -286,15 +292,30 @@ void Vessel::DeleteFlightPlan() {

void Vessel::RefreshPrediction() {
absl::MutexLock l(&prognosticator_lock_);
// The guard below ensures that the ephemeris will not be "forgotten before"
// the end of the psychohistory between now and the time when the
// prognosticator finishes the integration. This ensures that the ephemeris'
// |t_min| is never after the time that |FlowWithAdaptiveStep| tries to
// integrate.
// The guard will be destroyed either when the next set of parameters is
// created or when the prognostication has been computed.
// Note that we know that both |EventuallyForgetBefore| and
// |RefreshPrediction| are called on the main thread, therefore the ephemeris
// currently covers the last time of the psychohistory. Were this to change,
// this code might have to change.
prognosticator_parameters_ =
PrognosticatorParameters{psychohistory_->last().time(),
PrognosticatorParameters{Ephemeris<Barycentric>::Guard(ephemeris_),
psychohistory_->last().time(),
psychohistory_->last().degrees_of_freedom(),
prediction_adaptive_step_parameters_,
/*shutdown=*/false};
if (synchronous_) {
std::unique_ptr<DiscreteTrajectory<Barycentric>> prognostication;
Status const status = FlowPrognostication(*prognosticator_parameters_,
prognostication);
std::optional<PrognosticatorParameters> prognosticator_parameters;
std::swap(prognosticator_parameters, prognosticator_parameters_);
Status const status =
FlowPrognostication(std::move(*prognosticator_parameters),
prognostication);
SwapPrognostication(prognostication, status);
} else {
StartPrognosticatorIfNeeded();
@@ -442,60 +463,50 @@ Vessel::Vessel()

void Vessel::StartPrognosticatorIfNeeded() {
prognosticator_lock_.AssertHeld();
CHECK(prognosticator_parameters_);
if (!prognosticator_.joinable()) {
prognosticator_ =
std::thread(std::bind(&Vessel::RepeatedlyFlowPrognostication, this));
}
}

void Vessel::RepeatedlyFlowPrognostication() {
std::optional<PrognosticatorParameters> previous_prognosticator_parameters;
for (;;) {
// No point in going faster than 50 Hz.
std::chrono::steady_clock::time_point const wakeup_time =
std::chrono::steady_clock::now() + std::chrono::milliseconds(20);

// The thread is only started after the parameters have been set, so we
// should always find parameters here.
std::optional<PrognosticatorParameters> prognosticator_parameters;
{
absl::ReaderMutexLock l(&prognosticator_lock_);
CHECK(prognosticator_parameters_);
prognosticator_parameters = prognosticator_parameters_;
if (!prognosticator_parameters_) {
// No parameters, let's wait for them to appear.
continue;
}
std::swap(prognosticator_parameters, prognosticator_parameters_);
}

if (prognosticator_parameters->shutdown) {
break;
}

// Do not reflow if the parameters have not changed: the same causes would
// produce the same effects.
if (!previous_prognosticator_parameters ||
*previous_prognosticator_parameters != prognosticator_parameters) {
std::unique_ptr<DiscreteTrajectory<Barycentric>> prognostication;
Status const status = FlowPrognostication(*prognosticator_parameters,
prognostication);
{
absl::MutexLock l(&prognosticator_lock_);
SwapPrognostication(prognostication, status);
}
previous_prognosticator_parameters = prognosticator_parameters;
std::unique_ptr<DiscreteTrajectory<Barycentric>> prognostication;
Status const status =
FlowPrognostication(std::move(*prognosticator_parameters),
prognostication);
{
absl::MutexLock l(&prognosticator_lock_);
SwapPrognostication(prognostication, status);
}

std::this_thread::sleep_until(wakeup_time);
}
}

Status Vessel::FlowPrognostication(
PrognosticatorParameters const& prognosticator_parameters,
PrognosticatorParameters prognosticator_parameters,
std::unique_ptr<DiscreteTrajectory<Barycentric>>& prognostication) {
// This function may be run in a separate thread, and bad things will happen
// if |EventuallyForgetBefore| runs in parallel with it: the ephemeris'
// |t_min| may end up being after the time that |FlowWithAdaptiveStep| tries
// to integrate, causing various check failures.
Ephemeris<Barycentric>::Guard g(ephemeris_);

// The guard contained in |prognosticator_parameters| ensures that the |t_min|
// of the ephemeris doesn't move in this function.
prognostication = std::make_unique<DiscreteTrajectory<Barycentric>>();
prognostication->Append(
prognosticator_parameters.first_time,
@@ -532,7 +543,6 @@ void Vessel::SwapPrognostication(
prognosticator_lock_.AssertHeld();
if (status.error() != Error::CANCELLED) {
prognostication_.swap(prognostication);
prognosticator_status_ = status;
}
}

8 changes: 6 additions & 2 deletions ksp_plugin/vessel.hpp
Original file line number Diff line number Diff line change
@@ -182,6 +182,7 @@ class Vessel {

private:
struct PrognosticatorParameters {
Ephemeris<Barycentric>::Guard guard;
Instant first_time;
DegreesOfFreedom<Barycentric> first_degrees_of_freedom;
Ephemeris<Barycentric>::AdaptiveStepParameters adaptive_step_parameters;
@@ -204,7 +205,7 @@ class Vessel {
// Runs the integrator to compute the |prognostication_| based on the given
// parameters.
Status FlowPrognostication(
PrognosticatorParameters const& prognosticator_parameters,
PrognosticatorParameters prognosticator_parameters,
std::unique_ptr<DiscreteTrajectory<Barycentric>>& prognostication);

// Publishes the prognostication if the computation was not cancelled.
@@ -237,9 +238,12 @@ class Vessel {
std::set<PartId> kept_parts_;

mutable absl::Mutex prognosticator_lock_;
// This member only contains a value if |RefreshPrediction| has been called
// but the parameters have not been picked by the |prognosticator_|. It never
// contains a moved-from value, and is only read using |std::swap| to ensure
// that reading it clears it.
std::optional<PrognosticatorParameters> prognosticator_parameters_
GUARDED_BY(prognosticator_lock_);
Status prognosticator_status_ GUARDED_BY(prognosticator_lock_);
std::thread prognosticator_;

// See the comments in pile_up.hpp for an explanation of the terminology.
6 changes: 4 additions & 2 deletions ksp_plugin_test/vessel_test.cpp
Original file line number Diff line number Diff line change
@@ -254,7 +254,8 @@ TEST_F(VesselTest, Prediction) {
Velocity<Barycentric>({150.0 / 3.0 * Metre / Second,
60.0 * Metre / Second,
50.0 * Metre / Second}))),
Return(Status::OK)));
Return(Status::OK)))
.WillRepeatedly(Return(Status::OK));
EXPECT_CALL(
ephemeris_,
FlowWithAdaptiveStep(_, _, astronomy::J2000 + 2 * Second, _, _, _))
@@ -268,7 +269,8 @@ TEST_F(VesselTest, Prediction) {
Velocity<Barycentric>({140.0 / 3.0 * Metre / Second,
50.0 * Metre / Second,
40.0 * Metre / Second}))),
Return(Status::OK)));
Return(Status::OK)))
.WillRepeatedly(Return(Status::OK));

vessel_.PrepareHistory(astronomy::J2000);
// Polling for the integration to happen.
8 changes: 4 additions & 4 deletions physics/ephemeris.hpp
Original file line number Diff line number Diff line change
@@ -291,14 +291,14 @@ class Ephemeris {
explicit Guard(not_null<Ephemeris<Frame> const*> ephemeris);
~Guard();

// Move only.
Guard(Guard&&) = default;
Guard& operator=(Guard&&) = default;
// Move only. A moved-from |Guard| does not protect anything.
Guard(Guard&& other);
Guard& operator=(Guard&& other);
Guard(Guard const&) = delete;
Guard& operator=(Guard const&) = delete;

private:
not_null<Ephemeris<Frame> const*> const ephemeris_;
Ephemeris<Frame> const* ephemeris_;
Instant t_min_;
};

27 changes: 26 additions & 1 deletion physics/ephemeris_body.hpp
Original file line number Diff line number Diff line change
@@ -814,7 +814,32 @@ Ephemeris<Frame>::Guard::Guard(

template<typename Frame>
Ephemeris<Frame>::Guard::~Guard() {
ephemeris_->protector_->Unprotect(t_min_);
// |ephemeris_| may be null for a moved-from object.
if (ephemeris_ != nullptr) {
ephemeris_->protector_->Unprotect(t_min_);
}
}

template<typename Frame>
Ephemeris<Frame>::Guard::Guard(Guard&& other)
: ephemeris_(std::move(other.ephemeris_)),
t_min_(std::move(other.t_min_)) {
other.ephemeris_ = nullptr;
}

template<typename Frame>
typename Ephemeris<Frame>::Guard&
Ephemeris<Frame>::Guard::operator=(Guard&& other) {
if (this != &other) {
// |ephemeris_| may be null for a moved-from object.
if (ephemeris_ != nullptr) {
ephemeris_->protector_->Unprotect(t_min_);
}
ephemeris_ = std::move(other.ephemeris_);
t_min_ = std::move(other.t_min_);
other.ephemeris_ = nullptr;
}
return *this;
}

template<typename Frame>