Fixed performance issue #7854

[hvlad]

No description provided.

[aafemt]

I wonder why all this atomic juggling instead of obtaining UCalendar in constructor? TimeZoneDesc itself is created on demand and four of five usages requires calendar.

[asfernandes]

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

[hvlad]

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

[asfernandes]

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.

[hvlad]

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

[hvlad]

About performance, with patch I have following result:

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

[asfernandes]

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

[hvlad]

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.

[hvlad]

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.

[hvlad]

I wonder why all this atomic juggles instead of obtaining UCalendar in constructor? TimeZoneDesc itself is created on demand.

FIrst, all TimeZoneDesc's is created in TimeZoneStartup ctor.
Second, there are 637 items in timeZoneList currently, do you offer to create UCalendar for all of them ?
Last, define what is "juggles" and why you so afraid of it ?

BTW, even if TimeZoneDesc were created on demand, some synchronization would still be required.

[aafemt]

FIrst, all TimeZoneDesc's is created in TimeZoneStartup ctor.

Oops, you are right, I misread getDesc(). Why they create time zones that are never going to be used?..

[hvlad]

Why they create time zones that are never going to be used?..

Because it is cheap and simple and allows to avoid synchronization later.

[asfernandes]

I used your implementation as base for alternative implementation and had better results:

Original:
TEST                 DURATION 
================ ============ 
TZ -> TS                  161 
TS -> TZ                  183 
TS -> TS                   12 
TZ -> TZ                   11 

This PR:
TEST                 DURATION 
================ ============ 
TZ -> TS                   36 
TS -> TZ                   56 
TS -> TS                   12 
TZ -> TZ                   11 

https://github.com/FirebirdSQL/firebird/pull/7859
TEST                 DURATION 
================ ============ 
TZ -> TS                   31 
TS -> TZ                   36 
TS -> TS                   11 
TZ -> TZ                   12 

[hvlad]

I used your implementation as base for alternative implementation and had better results:

Afraid it will be not as good when run under contention: ucal_open() is much more costly than ucal_clone().
So far I didn't run mt-tests for it, though.

[asfernandes]

I used your implementation as base for alternative implementation and had better results:

Afraid it will be not as good when run under contention: ucal_open() is much more costly than ucal_clone(). So far I didn't run mt-tests for it, though.

Calendar is maintained in use for small times. In the end, it could hurt only benchmarks probably.