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.

Sign up for GitHub

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

Use std::shared_mutex in time_zone_impl.cc #304

Closed
wants to merge 1 commit into from
Closed

Use std::shared_mutex in time_zone_impl.cc #304

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 6 additions & 5 deletions src/time_zone_impl.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@
#include <deque>
#include <memory>
#include <mutex>
#include <shared_mutex>
#include <string>
#include <unordered_map>
#include <utility>
Expand All @@ -33,10 +34,10 @@ using TimeZoneImplByName =
TimeZoneImplByName* time_zone_map = nullptr;

// Mutual exclusion for time_zone_map.
std::mutex& TimeZoneMutex() {
std::shared_mutex& TimeZoneMutex() {
// 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;
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm sorry, but requiring C++17 for CCTZ is a non-starter (at least in the medium term).

return *time_zone_mutex;
}

Expand All @@ -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());
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks a lot for suggestions. I will think a bit more.

Copy link
Contributor

Choose a reason for hiding this comment

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

My suggestion would be to change that type from a string to a cctz::time_zone or absl::TimeZone

Thanks 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.

if (time_zone_map != nullptr) {
TimeZoneImplByName::const_iterator itr = time_zone_map->find(name);
if (itr != time_zone_map->end()) {
Expand All @@ -72,7 +73,7 @@ bool time_zone::Impl::LoadTimeZone(const std::string& name, time_zone* tz) {
std::unique_ptr<const Impl> new_impl(new Impl(name));

// Add the new time zone to the map.
std::lock_guard<std::mutex> lock(TimeZoneMutex());
std::unique_lock<std::shared_mutex> lock(TimeZoneMutex());
if (time_zone_map == nullptr) time_zone_map = new TimeZoneImplByName;
const Impl*& impl = (*time_zone_map)[name];
if (impl == nullptr) { // this thread won any load race
Expand All @@ -83,7 +84,7 @@ bool time_zone::Impl::LoadTimeZone(const std::string& name, time_zone* tz) {
}

void time_zone::Impl::ClearTimeZoneMapTestOnly() {
std::lock_guard<std::mutex> lock(TimeZoneMutex());
std::unique_lock<std::shared_mutex> lock(TimeZoneMutex());
if (time_zone_map != nullptr) {
// Existing time_zone::Impl* entries are in the wild, so we can't delete
// them. Instead, we move them to a private container, where they are
Expand Down