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

Fixed performance issue #7854 #7858

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Fixed performance issue #7854 #7858

Show file tree
Hide file tree
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
43 changes: 35 additions & 8 deletions src/common/TimeZoneUtil.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,8 @@
#include "../common/os/os_utils.h"
#include "unicode/ucal.h"

#include <atomic>

using namespace Firebird;

namespace
Expand All @@ -45,8 +47,19 @@ namespace
public:
TimeZoneDesc(MemoryPool& pool)
: asciiName(pool),
unicodeName(pool)
unicodeName(pool),
calendar(nullptr)
{
}

~TimeZoneDesc()
{
UCalendar* val = calendar.exchange(nullptr);
if (val)
{
auto& icuLib = Jrd::UnicodeUtil::getConversionICU();
icuLib.ucalClose(val);
}
}

public:
Expand All @@ -70,9 +83,26 @@ namespace
return unicodeName.begin();
}

UCalendar* getCalendar(const Jrd::UnicodeUtil::ConversionICU& icuLib, UErrorCode* err) const
{
UCalendar* val = calendar.load();
if (!val)
{
val = icuLib.ucalOpen(getUnicodeName(), -1, NULL, UCAL_GREGORIAN, err);
Copy link
Member

Choose a reason for hiding this comment

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

I'd get icuLib here instead of receive by argument.

Copy link
Member Author

Choose a reason for hiding this comment

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

I considered it first, but then found that icuLib is already present at all points where getCalendar() is used.

if (!val)
return val;

UCalendar* old = calendar.exchange(val);
if (old)
return old;
}
return icuLib.ucalClone(val, err);
Copy link
Member

Choose a reason for hiding this comment

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

ucal_clone is not documented to be thread-safe, so it should not be assumed to be.
I do also not know its performance.
I'd instead implement a pool of unused objects where concurrent usage may do open/close additional copies and single usage would be reverted to the pool after its usage.

Copy link
Member Author

Choose a reason for hiding this comment

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

I followed this docs

https://unicode-org.github.io/icu/userguide/icu/design.html#thread-safe-const-apis

https://unicode-org.github.io/icu/userguide/icu/design.html#clone-vs-open

Copy link
Member Author

Choose a reason for hiding this comment

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

About performance, with patch I have following result:

TEST DURATION
TZ -> TS 44
TS -> TZ 70
TS -> TS 13
TZ -> TZ 13

Copy link
Member

Choose a reason for hiding this comment

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

"However, you cannot use a reference to an open service object in two threads at the same time if either of them calls any non-const API."

ucal_clone is const, but ucal_setmillis is not.

In no place it says ucal_clone can be called concurrently with others non-const functions, so it's not safe.

Copy link
Member Author

Choose a reason for hiding this comment

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

ucal_clone is const by itself thus it could be called with no sync.
ucal_setmillis is non-const but it uses own private instance of UCalendar, cloned from existing one.

Copy link
Member Author

Choose a reason for hiding this comment

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

Also, the full cite is:

However, you cannot use a reference to an open service object in two threads at the same time if either of them calls any non-const API. An individual open service object is not thread-safe for concurrent “writes”. Rather, for non-const use, you must use the clone function to create a copy of the service you want and then pass this copy to the second thread. This procedure allows you to use the same service in different threads, but avoids any thread synchronization or deadlock problems.

}

private:
string asciiName;
Array<UChar> unicodeName;
mutable std::atomic<UCalendar*> calendar;
};
}

Expand Down Expand Up @@ -593,8 +623,7 @@ void TimeZoneUtil::extractOffset(const ISC_TIMESTAMP_TZ& timeStampTz, SSHORT* of

Jrd::UnicodeUtil::ConversionICU& icuLib = Jrd::UnicodeUtil::getConversionICU();

UCalendar* icuCalendar = icuLib.ucalOpen(
getDesc(timeStampTz.time_zone)->getUnicodeName(), -1, NULL, UCAL_GREGORIAN, &icuErrorCode);
UCalendar* icuCalendar = getDesc(timeStampTz.time_zone)->getCalendar(icuLib, &icuErrorCode);

if (!icuCalendar)
status_exception::raise(Arg::Gds(isc_random) << "Error calling ICU's ucal_open.");
Expand Down Expand Up @@ -702,8 +731,7 @@ void TimeZoneUtil::localTimeStampToUtc(ISC_TIMESTAMP_TZ& timeStampTz)

Jrd::UnicodeUtil::ConversionICU& icuLib = Jrd::UnicodeUtil::getConversionICU();

UCalendar* icuCalendar = icuLib.ucalOpen(
getDesc(timeStampTz.time_zone)->getUnicodeName(), -1, NULL, UCAL_GREGORIAN, &icuErrorCode);
UCalendar* icuCalendar = getDesc(timeStampTz.time_zone)->getCalendar(icuLib, &icuErrorCode);

if (!icuCalendar)
status_exception::raise(Arg::Gds(isc_random) << "Error calling ICU's ucal_open.");
Expand Down Expand Up @@ -771,8 +799,7 @@ bool TimeZoneUtil::decodeTimeStamp(const ISC_TIMESTAMP_TZ& timeStampTz, bool gmt
#endif
Jrd::UnicodeUtil::ConversionICU& icuLib = Jrd::UnicodeUtil::getConversionICU();

UCalendar* icuCalendar = icuLib.ucalOpen(
getDesc(timeStampTz.time_zone)->getUnicodeName(), -1, NULL, UCAL_GREGORIAN, &icuErrorCode);
UCalendar* icuCalendar = getDesc(timeStampTz.time_zone)->getCalendar(icuLib, &icuErrorCode);

if (!icuCalendar)
status_exception::raise(Arg::Gds(isc_random) << "Error calling ICU's ucal_open.");
Expand Down Expand Up @@ -1067,7 +1094,7 @@ TimeZoneRuleIterator::TimeZoneRuleIterator(USHORT aId, const ISC_TIMESTAMP_TZ& a
{
UErrorCode icuErrorCode = U_ZERO_ERROR;

icuCalendar = icuLib.ucalOpen(getDesc(id)->getUnicodeName(), -1, NULL, UCAL_GREGORIAN, &icuErrorCode);
icuCalendar = getDesc(id)->getCalendar(icuLib, &icuErrorCode);

if (!icuCalendar)
status_exception::raise(Arg::Gds(isc_random) << "Error calling ICU's ucal_open.");
Expand Down
1 change: 1 addition & 0 deletions src/common/unicode_util.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -446,6 +446,7 @@ class ImplementConversionICU : public UnicodeUtil::ConversionICU, BaseICU
getEntryPoint("ucal_getTZDataVersion", inModule, ucalGetTZDataVersion);
getEntryPoint("ucal_getDefaultTimeZone", inModule, ucalGetDefaultTimeZone);
getEntryPoint("ucal_open", inModule, ucalOpen);
getEntryPoint("ucal_clone", inModule, ucalClone);
getEntryPoint("ucal_close", inModule, ucalClose);
getEntryPoint("ucal_setAttribute", inModule, ucalSetAttribute);
getEntryPoint("ucal_setMillis", inModule, ucalSetMillis);
Expand Down
1 change: 1 addition & 0 deletions src/common/unicode_util.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -130,6 +130,7 @@ class UnicodeUtil
int32_t (U_EXPORT2* ucalGetDefaultTimeZone) (UChar* result, int32_t resultCapacity, UErrorCode* ec);
UCalendar* (U_EXPORT2* ucalOpen) (const UChar* zoneID, int32_t len, const char* locale, UCalendarType type,
UErrorCode* err);
UCalendar* (U_EXPORT2* ucalClone)(const UCalendar* cal, UErrorCode* status);
void (U_EXPORT2* ucalClose) (UCalendar* cal);
void (U_EXPORT2* ucalSetAttribute) (UCalendar* cal, UCalendarAttribute attr, int32_t newValue);
void (U_EXPORT2* ucalSetMillis) (UCalendar* cal, UDate dateTime, UErrorCode* err);
Expand Down