Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sampling period for computing the rolling average real time factor #758

Merged
merged 9 commits into from
May 8, 2024
2 changes: 1 addition & 1 deletion conanfile.py
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ def requirements(self):
self.tool_requires("cmake/[>=3.19]")
self.requires("fmilibrary/[~2.3]")
self.requires("libzip/[>=1.7 <1.10]") # 1.10 deprecates some functions we use
self.requires("ms-gsl/[>=3 <5]", transitive_headers=True)
self.requires("ms-gsl/[>=3 <5]", transitive_headers=True, transitive_libs=True)
davidhjp01 marked this conversation as resolved.
Show resolved Hide resolved
if self.options.proxyfmu:
self.requires("proxyfmu/0.3.2@osp/stable")
self.requires("boost/[~1.81]", transitive_headers=True, transitive_libs=True) # Required by Thrift
Expand Down
10 changes: 10 additions & 0 deletions include/cosim/timer.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,15 @@ struct real_time_config
* This value is used for monitoring purposes only.
*/
std::atomic<int> steps_to_monitor = 5;

/**
* A sampling period in second(s) used in the rolling average real time factor calculation.
davidhjp01 marked this conversation as resolved.
Show resolved Hide resolved
* This can be useful when simulation step size is small and a fixed value for `steps_to_monitor`
* would not compute an accurate rolling average real time factor.
* When the value is greater than zero, the real time factor is computed periodically using the
* specified time instead of the `steps_to_monitor` value.
*/
std::atomic<double> sampling_period_to_monitor = -1;
davidhjp01 marked this conversation as resolved.
Show resolved Hide resolved
};

} // namespace cosim
Expand All @@ -54,6 +63,7 @@ class hash<cosim::real_time_config>
boost::hash_combine(seed, v.real_time_simulation.load());
boost::hash_combine(seed, v.real_time_factor_target.load());
boost::hash_combine(seed, v.steps_to_monitor.load());
boost::hash_combine(seed, v.sampling_period_to_monitor.load());
return seed;
}
};
Expand Down
34 changes: 27 additions & 7 deletions src/cosim/timer.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@

#include <chrono>
#include <thread>

typedef std::chrono::steady_clock Time;
constexpr std::chrono::microseconds MIN_SLEEP(100);

Expand Down Expand Up @@ -37,6 +36,12 @@ class real_time_timer::impl
const auto newHash = std::hash<real_time_config>()(*config_);
if (newHash != configHashValue_) {
start(currentTime);
auto samping_period = config_->sampling_period_to_monitor.load();
davidhjp01 marked this conversation as resolved.
Show resolved Hide resolved
if (samping_period > 0) {
sampling_period_to_monitor_ = to_duration(samping_period);
} else {
sampling_period_to_monitor_ = std::nullopt;
}
configHashValue_ = newHash;
}
double rtfTarget = config_->real_time_factor_target.load();
Expand Down Expand Up @@ -72,21 +77,36 @@ class real_time_timer::impl
std::shared_ptr<real_time_config> config_;
size_t configHashValue_;
std::shared_ptr<real_time_metrics> metrics_;
std::optional<duration> sampling_period_to_monitor_ = std::nullopt;

void update_rolling_average_real_time_factor(
Time::time_point& currentTime,
time_point& currentSimulationTime,
const duration& elapsedRealTime)
{
const auto elapsedSimTime = currentSimulationTime - rtSimulationStartTime_;
metrics_->rolling_average_real_time_factor = elapsedSimTime.count() / (1.0 * elapsedRealTime.count());
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

elapsedSimTime and elapsedRealTime are two different std::chrono::duration types associated with different clocks. The former is associated with Time, which is an alias for std::chrono::steady_clock and represents real time, while the latter is associated with cosim::detail::clock, which is a clock defined to represent simulation time. Therefore, it might be fragile to use their count() values like this, because it assumes that their tick period is the same.

Two options:

  1. Insert std::duration_cast calls and cast them to a common type with the same tick period.
  2. Insert a compile-time check like static_assert(decltype(elapsedSimTime)::period == decltype(elapsedRealTime)::period);

I think I prefer the first one if it works, but there may be arguments in favour of the latter too.

Copy link
Member

@kyllingstad kyllingstad May 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see now that this is old code moved from elsewhere in the file. But then this is a good opportunity to fix it. ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added duration_cast as well as in update_rolling_average_real_time_factor.

Copy link
Contributor Author

@davidhjp01 davidhjp01 May 7, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After adding duration_cast (or some other updates), I see noticeable performance drops. Investigating on it..

The issue was due to my laptop, updated as suggested.

rtStartTime_ = currentTime;
rtSimulationStartTime_ = currentSimulationTime;
rtCounter_ = 0L;
}

void update_real_time_factor(Time::time_point currentTime, time_point currentSimulationTime)
{
const auto relativeSimTime = currentSimulationTime - simulationStartTime_;
const auto relativeRealTime = currentTime - startTime_;
metrics_->total_average_real_time_factor = relativeSimTime.count() / (1.0 * relativeRealTime.count());

if (rtCounter_ >= config_->steps_to_monitor.load()) {
const auto elapsedSimTime = currentSimulationTime - rtSimulationStartTime_;
if (sampling_period_to_monitor_.has_value()) {
const auto elapsedRealTime = currentTime - rtStartTime_;

if (elapsedRealTime > sampling_period_to_monitor_.value()) {
update_rolling_average_real_time_factor(currentTime, currentSimulationTime, elapsedRealTime);
}
} else if (rtCounter_ >= config_->steps_to_monitor.load()) {
const auto elapsedRealTime = currentTime - rtStartTime_;

metrics_->rolling_average_real_time_factor = elapsedSimTime.count() / (1.0 * elapsedRealTime.count());
rtStartTime_ = currentTime;
rtSimulationStartTime_ = currentSimulationTime;
rtCounter_ = 0L;
update_rolling_average_real_time_factor(currentTime, currentSimulationTime, elapsedRealTime);
}
rtCounter_++;
}
Expand Down