Fixing an issue with using map::insert() to update traffic jam and other review fixes.

routing/edge_estimator.cpp

@@ -4,10 +4,21 @@
 
 #include "std/algorithm.hpp"
 
-namespace routing
-{
 using namespace traffic;
 
+namespace
+{
+double CalcTrafficFactor(SpeedGroup speedGroup)
+{
+  double const percentage =
+      0.01 * static_cast<double>(kSpeedGroupThresholdPercentage[static_cast<size_t>(speedGroup)]);
+  CHECK_GREATER(percentage, 0.0, ("speedGroup:", speedGroup));
+  return 1.0 / percentage;
+}
+}  // namespace
+
+namespace routing
+{
 double constexpr kKMPH2MPS = 1000.0 / (60 * 60);
 
 inline double TimeBetweenSec(m2::PointD const & from, m2::PointD const & to, double speedMPS)
@@ -25,10 +36,10 @@ public:
 
   // EdgeEstimator overrides:
   void Start(MwmSet::MwmId const & mwmId) override;
+  void Finish() override;
   double CalcEdgesWeight(uint32_t featureId, RoadGeometry const & road, uint32_t pointFrom,
                          uint32_t pointTo) const override;
   double CalcHeuristic(m2::PointD const & from, m2::PointD const & to) const override;
-  void Finish() override;
 
 private:
   TrafficInfoGetter const & m_trafficGetter;
@@ -47,6 +58,11 @@ void CarEdgeEstimator::Start(MwmSet::MwmId const & mwmId)
   m_trafficInfo = m_trafficGetter.GetTrafficInfo(mwmId);
 }
 
+void CarEdgeEstimator::Finish()
+{
+  m_trafficInfo.reset();
+}
+
 double CarEdgeEstimator::CalcEdgesWeight(uint32_t featureId, RoadGeometry const & road,
                                          uint32_t pointFrom, uint32_t pointTo) const
 {
@@ -66,9 +82,7 @@ double CarEdgeEstimator::CalcEdgesWeight(uint32_t featureId, RoadGeometry const
       SpeedGroup const speedGroup =
           m_trafficInfo->GetSpeedGroup(TrafficInfo::RoadSegmentId(featureId, i, dir));
       CHECK_LESS(speedGroup, SpeedGroup::Count, ());
-      double const percentage =
-          0.01 * static_cast<double>(kSpeedGroupThresholdPercentage[static_cast<size_t>(speedGroup)]);
-      factor = 1.0 / percentage;
+      factor = CalcTrafficFactor(speedGroup);
     }
     result += factor * TimeBetweenSec(road.GetPoint(i), road.GetPoint(i + 1), speedMPS);
   }
@@ -80,11 +94,6 @@ double CarEdgeEstimator::CalcHeuristic(m2::PointD const & from, m2::PointD const
 {
   return TimeBetweenSec(from, to, m_maxSpeedMPS);
 }
-
-void CarEdgeEstimator::Finish()
-{
-  m_trafficInfo.reset();
-}
 }  // namespace
 
 namespace routing

routing/edge_estimator.hpp

@@ -20,17 +20,27 @@ public:
   virtual ~EdgeEstimator() = default;
 
   virtual void Start(MwmSet::MwmId const & mwmId) = 0;
-
+  virtual void Finish() = 0;
   virtual double CalcEdgesWeight(uint32_t featureId, RoadGeometry const & road, uint32_t pointFrom,
                                  uint32_t pointTo) const = 0;
   virtual double CalcHeuristic(m2::PointD const & from, m2::PointD const & to) const = 0;
 
-  virtual void Finish() = 0;
 
   static shared_ptr<EdgeEstimator> CreateForCar(IVehicleModel const & vehicleModel,
                                                 traffic::TrafficInfoGetter const & getter);
+};
 
-protected:
-  shared_ptr<traffic::TrafficInfo> m_trafficInfo;
+class EstimatorGuard final
+{
+public:
+  EstimatorGuard(MwmSet::MwmId const & mwmId, EdgeEstimator & estimator) : m_estimator(estimator)
+  {
+    m_estimator.Start(mwmId);
+  }
+
+  ~EstimatorGuard() { m_estimator.Finish(); }
+
+private:
+  EdgeEstimator & m_estimator;
 };
 }  // namespace routing

routing/routing_session.cpp

@@ -49,6 +49,25 @@ uint32_t constexpr kMinimumETASec = 60;
 
 namespace routing
 {
+void TrafficCache::Set(TrafficInfo && info)
+{
+  MwmSet::MwmId const mwmId = info.GetMwmId();
+  m_trafficInfo[mwmId] = make_shared<TrafficInfo>(move(info));
+}
+
+void TrafficCache::Remove(MwmSet::MwmId const & mwmId) { m_trafficInfo.erase(mwmId); }
+
+shared_ptr<TrafficInfo> TrafficCache::Get(MwmSet::MwmId const & mwmId) const
+{
+  auto it = m_trafficInfo.find(mwmId);
+
+  if (it == m_trafficInfo.cend())
+    return shared_ptr<TrafficInfo>();
+  return it->second;
+}
+
+void TrafficCache::Clear() { m_trafficInfo.clear(); }
+
 RoutingSession::RoutingSession()
   : m_router(nullptr)
   , m_route(make_shared<Route>(string()))
@@ -585,32 +604,28 @@ void RoutingSession::OnTrafficEnabled(bool enable)
   threads::MutexGuard guard(m_routingSessionMutex);
   UNUSED_VALUE(guard);
   if (!enable)
-    m_trafficInfo.clear();
+    m_trafficCache.Clear();
 }
 
 void RoutingSession::OnTrafficInfoAdded(TrafficInfo && info)
 {
   threads::MutexGuard guard(m_routingSessionMutex);
   UNUSED_VALUE(guard);
-  m_trafficInfo.insert(make_pair(info.GetMwmId(), make_shared<TrafficInfo>(move(info))));
+  m_trafficCache.Set(move(info));
 }
 
 void RoutingSession::OnTrafficInfoRemoved(MwmSet::MwmId const & mwmId)
 {
   threads::MutexGuard guard(m_routingSessionMutex);
   UNUSED_VALUE(guard);
-  m_trafficInfo.erase(mwmId);
+  m_trafficCache.Remove(mwmId);
 }
 
 shared_ptr<traffic::TrafficInfo> RoutingSession::GetTrafficInfo(MwmSet::MwmId const & mwmId) const
 {
   threads::MutexGuard guard(m_routingSessionMutex);
   UNUSED_VALUE(guard);
-  auto it = m_trafficInfo.find(mwmId);
-
-  if (it == m_trafficInfo.cend())
-    return shared_ptr<TrafficInfo>();
-  return it->second;
+  return m_trafficCache.Get(mwmId);
 }
 
 string DebugPrint(RoutingSession::State state)

routing/routing_session.hpp

@@ -38,6 +38,18 @@ struct SpeedCameraRestriction
   SpeedCameraRestriction() : m_index(0), m_maxSpeedKmH(numeric_limits<uint8_t>::max()) {}
 };
 
+class TrafficCache
+{
+public:
+  void Set(traffic::TrafficInfo && info);
+  void Remove(MwmSet::MwmId const & mwmId);
+  shared_ptr<traffic::TrafficInfo> Get(MwmSet::MwmId const & mwmId) const;
+  void Clear();
+
+private:
+  map<MwmSet::MwmId, shared_ptr<traffic::TrafficInfo>> m_trafficInfo;
+};
+
 class RoutingSession : public traffic::TrafficObserver, public traffic::TrafficInfoGetter
 {
   friend void UnitTest_TestFollowRoutePercentTest();
@@ -191,7 +203,7 @@ private:
   double GetCompletionPercent() const;
 
 private:
-  map<MwmSet::MwmId, shared_ptr<traffic::TrafficInfo>> m_trafficInfo;
+  TrafficCache m_trafficCache;
   unique_ptr<AsyncRouter> m_router;
   shared_ptr<Route> m_route;
   atomic<State> m_state;

routing/routing_tests/applying_traffic_test.cpp

@@ -4,6 +4,7 @@
 #include "routing/geometry.hpp"
 #include "routing/index_graph.hpp"
 #include "routing/index_graph_starter.hpp"
+#include "routing/routing_session.hpp"
 
 #include "routing/routing_tests/index_graph_tools.hpp"
 
@@ -85,22 +86,24 @@ unique_ptr<IndexGraph> BuildXXGraph(shared_ptr<EdgeEstimator> estimator)
   return graph;
 }
 
-class TrafficInfoGetterTest : public TrafficInfoGetter
+class TrafficInfoGetterForTesting : public TrafficInfoGetter
 {
 public:
-  TrafficInfoGetterTest(shared_ptr<TrafficInfo> trafficInfo = shared_ptr<TrafficInfo>())
-    : m_trafficInfo(trafficInfo) {}
-
-  // TrafficInfoGetter overrides:
-  shared_ptr<traffic::TrafficInfo> GetTrafficInfo(MwmSet::MwmId const &) const override
+  TrafficInfoGetterForTesting(TrafficInfo && trafficInfo)
   {
-    return m_trafficInfo;
+    m_trafficCache.Set(move(trafficInfo));
   }
 
-  void UpdateTrafficInfo(shared_ptr<TrafficInfo> trafficInfo) { m_trafficInfo = trafficInfo; }
+  // TrafficInfoGetter overrides:
+  shared_ptr<traffic::TrafficInfo> GetTrafficInfo(MwmSet::MwmId const & mwmId) const override
+  {
+    return m_trafficCache.Get(mwmId);
+  }
+
+  void UpdateTrafficInfo(TrafficInfo && trafficInfo) { m_trafficCache.Set(move(trafficInfo)); }
 
 private:
-  shared_ptr<TrafficInfo> m_trafficInfo;
+  TrafficCache m_trafficCache;
 };
 
 class ApplyingTrafficTest
@@ -110,7 +113,7 @@ public:
 
   void SetEstimator(TrafficInfo::Coloring && coloring)
   {
-    m_trafficGetter = make_unique<TrafficInfoGetterTest>(make_shared<TrafficInfo>(move(coloring)));
+    m_trafficGetter = make_unique<TrafficInfoGetterForTesting>(move(coloring));
     m_estimator = EdgeEstimator::CreateForCar(*make_shared<CarModelFactory>()->GetVehicleModel(),
                                               *m_trafficGetter);
   }
@@ -119,11 +122,11 @@ public:
 
   void UpdateTrafficInfo(TrafficInfo::Coloring && coloring)
   {
-    m_trafficGetter->UpdateTrafficInfo(make_shared<TrafficInfo>(move(coloring)));
+    m_trafficGetter->UpdateTrafficInfo(TrafficInfo(move(coloring)));
   }
 
 private:
-  unique_ptr<TrafficInfoGetterTest> m_trafficGetter;
+  unique_ptr<TrafficInfoGetterForTesting> m_trafficGetter;
   shared_ptr<EdgeEstimator> m_estimator;
 };
 
@@ -194,7 +197,6 @@ UNIT_CLASS_TEST(ApplyingTrafficTest, XXGraph_ChangingTraffic)
 {
   // No trafic at all.
   SetEstimator({});
-
   unique_ptr<IndexGraph> graph = BuildXXGraph(GetEstimator());
   IndexGraphStarter starter(*graph, RoadPoint(1, 0) /* start */, RoadPoint(6, 1) /* finish */);
   vector<m2::PointD> const noTrafficGeom = {{2 /* x */, 0 /* y */}, {1, 1}, {2, 2}, {3, 3}};
@@ -211,5 +213,22 @@ UNIT_CLASS_TEST(ApplyingTrafficTest, XXGraph_ChangingTraffic)
   vector<m2::PointD> const heavyF3Geom = {{2 /* x */, 0 /* y */}, {3, 0}, {3, 1}, {2, 2}, {3, 3}};
   TestRouteGeometry(starter, AStarAlgorithm<IndexGraphStarter>::Result::OK, heavyF3Geom);
   GetEstimator()->Finish();
+
+  // Overloading traffic jam on F3. Middle traffic (SpeedGroup::G3) on F1, F3, F4, F7 and F8.
+  TrafficInfo::Coloring coloringMiddleF1F3F4F7F8 = {
+      {{1 /* feature id */, 0 /* segment id */, TrafficInfo::RoadSegmentId::kForwardDirection},
+       SpeedGroup::G3},
+      {{3 /* feature id */, 0 /* segment id */, TrafficInfo::RoadSegmentId::kForwardDirection},
+       SpeedGroup::G3},
+      {{4 /* feature id */, 0 /* segment id */, TrafficInfo::RoadSegmentId::kForwardDirection},
+       SpeedGroup::G3},
+      {{7 /* feature id */, 0 /* segment id */, TrafficInfo::RoadSegmentId::kForwardDirection},
+       SpeedGroup::G3},
+      {{8 /* feature id */, 0 /* segment id */, TrafficInfo::RoadSegmentId::kForwardDirection},
+       SpeedGroup::G3}};
+  UpdateTrafficInfo(move(coloringMiddleF1F3F4F7F8));
+  GetEstimator()->Start(MwmSet::MwmId());
+  TestRouteGeometry(starter, AStarAlgorithm<IndexGraphStarter>::Result::OK, noTrafficGeom);
+  GetEstimator()->Finish();
 }
 }  // namespace

routing/single_mwm_router.cpp

@@ -44,20 +44,6 @@ vector<Junction> ConvertToJunctions(IndexGraphStarter & starter, vector<Joint::I
 
   return junctions;
 }
-
-class EstimatorGuard
-{
-public:
-  EstimatorGuard(MwmSet::MwmId const & mwmId, EdgeEstimator & estimator) : m_estimator(estimator)
-  {
-    m_estimator.Start(mwmId);
-  }
-
-  ~EstimatorGuard() { m_estimator.Finish(); }
-
-private:
-  EdgeEstimator & m_estimator;
-};
 }  // namespace
 
 namespace routing

traffic/traffic_info.cpp

@@ -77,6 +77,13 @@ TrafficInfo::TrafficInfo(MwmSet::MwmId const & mwmId, int64_t currentDataVersion
   , m_currentDataVersion(currentDataVersion)
 {}
 
+TrafficInfo::TrafficInfo(TrafficInfo && info)
+  : m_coloring(move(info.m_coloring))
+  , m_mwmId(info.m_mwmId)
+  , m_availability(info.m_availability)
+  , m_currentDataVersion(info.m_currentDataVersion)
+{}
+
 bool TrafficInfo::ReceiveTrafficData()
 {
   auto const & info = m_mwmId.GetInfo();

traffic/traffic_info.hpp

@@ -68,7 +68,7 @@ public:
 
   TrafficInfo(MwmSet::MwmId const & mwmId, int64_t currentDataVersion);
 
-  TrafficInfo(TrafficInfo && info) : m_coloring(move(info.m_coloring)), m_mwmId(info.m_mwmId) {}
+  TrafficInfo(TrafficInfo && info);
 
   // For testing only.
   TrafficInfo(Coloring && coloring) : m_coloring(move(coloring)) {}