Improve Timezone Diff Performance #2316

kevinkreiser · 2020-04-12T03:31:37Z

As noted in: #2278 (comment) our use of timezone diffing is not performant. In routes that cross multiple timezones and are particularly long we spend the vast majority of our CPU just computing timezone differences. In the linked issue above we spend about 14 seconds of a 15 second route calculation just doing timezone differencing..

This PR implements a caching strategy on top of timezone info look up so that as a given route progresses we dont need to construct timezone information to do the differencing. A first proof of concept laid out here: HowardHinnant/date#569.

~~TODO: integrate the POC into our datetime utility which does the differencing~~
~~TODO: performance test~~

as a test i disabled the distance limitation on timedep_forward and ran a route from Minsk to Aberdeen. with the old code this takes 15 seconds to complete on mondern hardware. with the timezone caching this comes down to 1.5 seconds. of course bidirectional astar is only 300ms. but we are working on getting timedependence in there

kevinkreiser · 2020-04-12T03:33:46Z

src/baldr/datetime.cc

-  auto duration = std::chrono::duration_cast<std::chrono::seconds>(origin.get_local_time() -
-                                                                   dest.get_local_time());
-  if (origin.get_info().offset < dest.get_info().offset) {
-    return std::abs(duration.count());
-  } else {
-    return -1 * std::abs(duration.count());
-  }


under the hood get_local_time also calls get_info. get_info is quite a heavy call. it turns out you can use the info returned from it to do the timezone offset directly so just this small refactor ends up cutting our long time dependent routes response time in half.

kevinkreiser · 2020-04-12T03:40:38Z

src/baldr/tz_alt.cpp

@@ -114,6 +114,7 @@
 #include <cctype>
 #include <cstdlib>
 #include <cstring>
+#include <cwchar>


long story here. i know there was a thread about this when i did it originally but instead of taking the implementation inside of the date library directly. we copy it and modify it to not need to use CURL/filesystem to grab timezone database information but instead we package that data as strings with the build (as we do many things). this allows us to ship the library with all of the data it needs to run (other than the actual tiles).

anyway i updated the submodule to the latest version and in doing so had to redo my hacks for loading tzdb from strings.

the other thing this does is it ensure every platform has the same idea about what timezones exist where. faster, per-platform implementations exist but they dont agree that ET is index 109 for example which is a problem for us because we bake the timezone index into the tile data. so by making the data static we ensure everyone agrees on timezones.

kevinkreiser · 2020-04-12T03:41:35Z

valhalla/baldr/datetime.h

@@ -31,7 +32,7 @@ struct tz_db_t {
  const date::time_zone* from_index(size_t index) const;

 protected:
-  std::vector<std::string> names;
+  std::unordered_map<std::string, size_t> names;


small refactor to use a hash map to look up the index from the name rather than std::find. probably not needed but if we do this alot somewhere it will be way faster than scanning 300+ entries

This reverts commit d454bd2.

kevinkreiser · 2020-04-14T12:53:57Z

src/baldr/datetime.cc

@@ -13,28 +13,59 @@
 #include "midgard/logging.h"
 #include "midgard/util.h"

+namespace {
+// use a cache to store already constructed sys_info's since they aren't cheap


this is where the speed up is had. more info about how this works can be found here: HowardHinnant/date#569 (comment)

kevinkreiser · 2020-04-14T12:56:43Z

src/thor/route_action.cc

@@ -232,6 +232,7 @@ thor::PathAlgorithm* thor_worker_t::get_path_algorithm(const std::string& routet
                                                       const valhalla::Location& destination) {
  // Have to use multimodal for transit based routing
  if (routetype == "multimodal" || routetype == "transit") {
+    valhalla::midgard::logging::Log("algorithm::multimodal ", " [ANALYTICS] ");


it seemed crazy to me that we dont log the algorithm that we use. in fact this was the source of much frustration for me. i wanted to show that this implementation speeds up timedep forward but everytime i tried a route from say minsk to london it was the same speed as bidir astar and the reason for that was...

kevinkreiser · 2020-04-14T12:57:19Z

src/thor/route_action.cc

@@ -242,6 +243,7 @@ thor::PathAlgorithm* thor_worker_t::get_path_algorithm(const std::string& routet
    PointLL ll1(origin.ll().lng(), origin.ll().lat());
    PointLL ll2(destination.ll().lng(), destination.ll().lat());
    if (ll1.Distance(ll2) < max_timedep_distance) {


this line... if the route is more than 500km then it wont do time dependent but instead falls back to bidirastar.

kevinkreiser · 2020-04-14T12:57:58Z

test/datetime.cc

@@ -583,9 +583,56 @@ TEST(DateTime, TestSecondOfWeek) {
  EXPECT_EQ(a, e) << "Wrong second of week";
 }

+TEST(DateTime, DiffCaching) {
+  // no cache


first we check that the changes work even with out cache but checking the time difference between NY and LA to be -3 hours

kevinkreiser · 2020-04-14T12:58:40Z

test/datetime.cc

+  // for a really long route that crosses many timezone we end up doing a lot of tz diffing
+  // the tuple is: number of calls, origin tz index, destination tz index
+  // 0 is reserved for no timezone so we shift down by one to use them with the tzdb
+  std::vector<std::tuple<int, int, int>> test_cases = {


then we take what is a play by play of all the calls to the timezone diff method for a very long route through africa starting in europe

kevinkreiser · 2020-04-14T12:59:42Z

test/datetime.cc

 } // namespace

 int main(int argc, char* argv[]) {
+  // make this whole thing bail if it doesnt finish fast
+  alarm(10);


even on a really crazy machine the implementation without cache takes something like 15 seconds. with the cache the implementation is subsecond.

Could this github comment be put as the code comment? I think it describes the intent of the alarm a lot better than the short code comment.

purew · 2020-04-14T13:05:43Z

src/baldr/datetime.cc

+           const date::time_zone* tz,
+           std::unordered_map<const date::time_zone*, std::vector<date::sys_info>>& cache) {
+  // check if we have anything for this timezone in the cache
+  auto tz_it = cache.find(tz);


Is it ok to use a pointer as the cache key? Is each timezone only instantiated once?

They are statically initialized in a singleton

purew · 2020-04-14T13:06:38Z

src/baldr/datetime.cc

+  auto tz_it = cache.find(tz);
+  if (tz_it != cache.cend()) {
+    // we have something in the cache, see if one of the infos has the particular time point in range
+    auto st = date::floor<std::chrono::seconds>(tp.get_sys_time());


Could st be named something more verbose?

I was just following the patterns of the library it comes from. Tp=timepoint and st=system time. I think it's pretty clear from the function call get sys time no?

purew · 2020-04-14T13:12:50Z

src/baldr/datetime.cc

+    auto st = date::floor<std::chrono::seconds>(tp.get_sys_time());
+    auto info_it =
+        std::find_if(tz_it->second.begin(), tz_it->second.end(),
+                     [&st](const date::sys_info& info) { return info.begin <= st && st < info.end; });


I assume the array in the map is usually 1-2 elements long? There wouldn't be any point in sorting it according to begin time which would allow a binary search?

hahah i had that as a TODO but you are exactly right. there is basically no benefit. the only time the array isnt a single entry long is if the route crosses over daylight savings time

purew · 2020-04-14T13:28:55Z

src/baldr/tz_alt.cpp

@@ -526,7 +533,7 @@ native_to_standard_timezone_name(const std::string& native_tz_name,
 }

 // Parse this XML file:
-// http://unicode.org/repos/cldr/trunk/common/supplemental/windowsZones.xml
+// https://raw.githubusercontent.com/unicode-org/cldr/master/common/supplemental/windowsZones.xml


Is the build fetching this at build-time?

no. all of the strings needed for all of the target platforms crammed into headers and read directly from memory. thats what my previous comment on this file was talking about. although tbh im not sure that windows has the same indexing scheme as non windows platforms (and if not (i think its not) its a long standing bug that we'll have to tackle in another)

purew

Nice speedup! LGTM

* master: bidir a* elapsed time/cost tracking has a bug and its.... partial distance strikes again! (#2323) Fixes for Windows (#2314) fix local date shifting to be without timezone and add test (#2312) Improve Timezone Diff Performance (#2316) Missed changelog (#2318) Fix break/continue typo in search filtering (#2317) Approach, multi-cue, and length updates (#2313) Added default tile getter interface (#2309) Use flow mask when considering access for live traffic (#2311) Fix typos (#2310) Add missing declaration for helper function. (#2304) Guidance views - use changeset for data_id and MatchGuidanceViewJunctions fix (#2303)

kevinkreiser added 2 commits April 10, 2020 13:45

cleanup find for name to index lookup

911041e

complete the upgrade to latest date library add diff test

a76ae1b

kevinkreiser commented Apr 12, 2020

View reviewed changes

kevinkreiser changed the title ~~Improve Timezone Diff Performance~~ WIP: Improve Timezone Diff Performance Apr 12, 2020

kevinkreiser added 5 commits April 13, 2020 14:59

try not using special implementations of tz data

d454bd2

Revert "try not using special implementations of tz data"

faf82d1

This reverts commit d454bd2.

we should know what algorithm is run

ee7fa1b

allow a cache to be used with timezone differencing

982820f

add test with and without cache for tz diff

33898c3

kevinkreiser changed the title ~~WIP: Improve Timezone Diff Performance~~ Improve Timezone Diff Performance Apr 14, 2020

kevinkreiser added 2 commits April 14, 2020 08:52

update changelog

6626265

Merge branch 'master' into date_hacking

293e53b

kevinkreiser requested review from purew and danpaz April 14, 2020 12:52

kevinkreiser commented Apr 14, 2020

View reviewed changes

purew reviewed Apr 14, 2020

View reviewed changes

kevinkreiser added 2 commits April 14, 2020 09:16

try to please older compiler

b166611

Merge remote-tracking branch 'origin/date_hacking' into date_hacking

4c14074

purew reviewed Apr 14, 2020

View reviewed changes

forgot to remove arg from windows string loader

c78dc13

purew approved these changes Apr 14, 2020

View reviewed changes

kevinkreiser merged commit 6822ba0 into master Apr 14, 2020

purew deleted the date_hacking branch April 14, 2020 19:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve Timezone Diff Performance #2316

Improve Timezone Diff Performance #2316

kevinkreiser commented Apr 12, 2020 •

edited

kevinkreiser Apr 12, 2020

kevinkreiser Apr 12, 2020

kevinkreiser Apr 14, 2020

kevinkreiser Apr 12, 2020

kevinkreiser Apr 14, 2020

kevinkreiser Apr 14, 2020

kevinkreiser Apr 14, 2020

kevinkreiser Apr 14, 2020

kevinkreiser Apr 14, 2020

kevinkreiser Apr 14, 2020

purew Apr 14, 2020

purew Apr 14, 2020

kevinkreiser Apr 14, 2020

purew Apr 14, 2020

kevinkreiser Apr 14, 2020

purew Apr 14, 2020

kevinkreiser Apr 14, 2020

purew Apr 14, 2020

kevinkreiser Apr 14, 2020

purew left a comment

Improve Timezone Diff Performance #2316

Improve Timezone Diff Performance #2316

Conversation

kevinkreiser commented Apr 12, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

purew left a comment

Choose a reason for hiding this comment

kevinkreiser commented Apr 12, 2020 •

edited