Use std::shared_mutex in time_zone_impl.cc #304
Conversation
|
I just saw the thread in #212 so I guess the suggestion is still to use |
Right. That's why you can use the returned
Yes. But you might also consider why you're calling |
| // This mutex is intentionally "leaked" to avoid the static deinitialization | ||
| // order fiasco (std::mutex's destructor is not trivial on many platforms). | ||
| static std::mutex* time_zone_mutex = new std::mutex; | ||
| static std::shared_mutex* time_zone_mutex = new std::shared_mutex; |
There was a problem hiding this comment.
I'm sorry, but requiring C++17 for CCTZ is a non-starter (at least in the medium term).
| @@ -58,7 +59,7 @@ bool time_zone::Impl::LoadTimeZone(const std::string& name, time_zone* tz) { | |||
|
|
|||
| // Check whether the time zone has already been loaded. | |||
| { | |||
| std::lock_guard<std::mutex> lock(TimeZoneMutex()); | |||
| std::shared_lock<std::shared_mutex> lock(TimeZoneMutex()); | |||
There was a problem hiding this comment.
If you are seeing contention on this mutex, which is only held over an unordered_map lookup, then I can only suggest that you are calling cctz::load_time_zone() way too much. Typically you would only load a time zone once, and then use the returned cctz::time_zone value after that.
There was a problem hiding this comment.
The problem is that our functions will take an optional timezone parameter (string) from the user input. We need to have a mechanism to load it once and store it somewhere (we probably want to avoid using lock again on a map). I am thinking to have a static map <name, timezone> to pre-load all time zones which is not ideal and seems to waste space since most timezones are not used.
There was a problem hiding this comment.
The problem is that our functions will take an optional timezone parameter (string) from the user input. We need to have a mechanism to load it once and store it somewhere (we probably want to avoid using lock again on a map).
My suggestion would be to change that type from a string to a cctz::time_zone or absl::TimeZone as they give you exactly that mechanism. Then the user would be able to load their timezone(s) of interest only once.
I am thinking to have a static map <name, timezone> to pre-load all time zones which is not ideal and seems to waste space since most timezones are not used.
Alternately, introduce your own "timezone" type and factory (if you don't want cctz/absl types in your vocabulary). The type would have an accessor for its held cctz/absl zone, and the by-string factory would maintain the map and only load requested zones, not all of them. (Note that "all time zones" is not even a supported concept.)
Or, you could keep your string parameter, but have some variety of internal static map, which, again, is not pre-loaded but rather filled on demand. In each case, the thread safety requirements of the map would be determined by the thread safety of your interfaces.
There was a problem hiding this comment.
Thanks a lot for suggestions. I will think a bit more.
There was a problem hiding this comment.
My suggestion would be to change that type from a string to a
cctz::time_zoneorabsl::TimeZoneThanks a lot for suggestions. I will think a bit more.
Not only is that simpler and gives the user control over when zones are loaded, it would also save you from having to deal with load failures.
Good luck.
|
Close this. As suggested, we should avoid calling |
We have observed high lock contention issues (consume a lot of CPUs) when calling absl::LoadTimeZone. We wonder if it is better to use
std::shared_mutexto allow multiple readers toaccess the time zone map concurrently. The entries in the time zone map are never modified after they are initialized.