[BZ,#16145] Reduce lock contention in __tz_convert()

This patch is an "easy win" partial fix for BZ #16145, which notes
the heavy contention on tzset_lock when multiple threads are converting
times with localtime_r().

In __tz_convert(), the lock does not need to be held after
__tzfile_compute() / __tz_compute() have been called, so we can move the
unlock up.  At this point there is still significant work to be done in
__offtime(), so we see some improvement (in my testing with 8 cores
banging on localtime_r(), ~20% improvement in throughput).

	[BZ #16145] (partial fix)
	* time/tzset.c (__tz_convert): Unlock tzset_lock earlier
	to reduce lock contention.

Pinging this for review.  (Should I be CCing a particular reviewer?)

    - Kevin

On Thu, Feb 12, 2015 at 12:37:19AM +1100, Kevin Easton wrote:
> This patch is an "easy win" partial fix for BZ #16145, which notes
> the heavy contention on tzset_lock when multiple threads are converting
> times with localtime_r().
> 
> In __tz_convert(), the lock does not need to be held after
> __tzfile_compute() / __tz_compute() have been called, so we can move the
> unlock up.  At this point there is still significant work to be done in
> __offtime(), so we see some improvement (in my testing with 8 cores
> banging on localtime_r(), ~20% improvement in throughput).
> 
> 	[BZ #16145] (partial fix)
> 	* time/tzset.c (__tz_convert): Unlock tzset_lock earlier
> 	to reduce lock contention.
> 
> diff --git a/time/tzset.c b/time/tzset.c
> index 8bc7a2e..82324ca 100644
> --- a/time/tzset.c
> +++ b/time/tzset.c
> @@ -644,6 +644,8 @@ __tz_convert (const time_t *timer, int use_localtime, struct tm *tp)
>        leap_extra_secs = 0;
>      }
> 
> +  __libc_lock_unlock (tzset_lock);
> +
>    if (tp)
>      {
>        if (! use_localtime)
> @@ -659,8 +661,6 @@ __tz_convert (const time_t *timer, int use_localtime, struct tm *tp)
>         tp = NULL;
>      }
> 
> -  __libc_lock_unlock (tzset_lock);
> -
>    return tp;
>  }
>

On 02/19/2015 08:15 AM, Kevin Easton wrote:
> Pinging this for review.  (Should I be CCing a particular reviewer?)

No, there is no notion of subsystem maintainer. Any of the established
maintainers could review your patch and commit it.

c.

On 12 Feb 2015 00:37, Kevin Easton wrote:
> This patch is an "easy win" partial fix for BZ #16145, which notes
> the heavy contention on tzset_lock when multiple threads are converting
> times with localtime_r().
> 
> In __tz_convert(), the lock does not need to be held after
> __tzfile_compute() / __tz_compute() have been called, so we can move the
> unlock up.  At this point there is still significant work to be done in
> __offtime(), so we see some improvement (in my testing with 8 cores
> banging on localtime_r(), ~20% improvement in throughput).

my reading of __offtime is that it only operates on its arguments (or const 
global data like __mon_yday).  it also looks expensive, so maybe we should 
hoist the remaining call outside of holding the lock ?  can you see if that'd 
have any measurable improvement ?

  ...
  int offtime = 0;
  if (!__use_tzfile)
    offtime = __offtime (...);

  __libc_lock_lock (tzset_lock);
  ...
  if (__use_tzfile)
    __tzfile_compute (*timer, use_localtime, &leap_correction,
                      &leap_extra_secs, tp);
  else
    { 
      if (!offtime)  // modified this one line.
        tp = NULL;
      else
        __tz_compute (*timer, tp, use_localtime);
      leap_correction = 0L;
      leap_extra_secs = 0;
    }

  __libc_lock_unlock (tzset_lock);

at any rate, this patch as-is lgtm
-mike

On 24 Feb 2015 00:01, Mike Frysinger wrote:
> On 12 Feb 2015 00:37, Kevin Easton wrote:
> > This patch is an "easy win" partial fix for BZ #16145, which notes
> > the heavy contention on tzset_lock when multiple threads are converting
> > times with localtime_r().
> > 
> > In __tz_convert(), the lock does not need to be held after
> > __tzfile_compute() / __tz_compute() have been called, so we can move the
> > unlock up.  At this point there is still significant work to be done in
> > __offtime(), so we see some improvement (in my testing with 8 cores
> > banging on localtime_r(), ~20% improvement in throughput).
> 
> my reading of __offtime is that it only operates on its arguments (or const 
> global data like __mon_yday).  it also looks expensive, so maybe we should 
> hoist the remaining call outside of holding the lock ?  can you see if that'd 
> have any measurable improvement ?

i've pushed your patch as-is, but if you could vet this idea (since you 
already have benchmarks set up), that'd be great ...
-mike

On Tue, Feb 24, 2015 at 12:01:58AM -0500, Mike Frysinger wrote:
> On 12 Feb 2015 00:37, Kevin Easton wrote:
> > This patch is an "easy win" partial fix for BZ #16145, which notes
> > the heavy contention on tzset_lock when multiple threads are converting
> > times with localtime_r().
> > 
> > In __tz_convert(), the lock does not need to be held after
> > __tzfile_compute() / __tz_compute() have been called, so we can move the
> > unlock up.  At this point there is still significant work to be done in
> > __offtime(), so we see some improvement (in my testing with 8 cores
> > banging on localtime_r(), ~20% improvement in throughput).
> 
> my reading of __offtime is that it only operates on its arguments (or const 
> global data like __mon_yday).  it also looks expensive, so maybe we should 
> hoist the remaining call outside of holding the lock ?  can you see if that'd 
> have any measurable improvement ?
> 
>   ...
>   int offtime = 0;
>   if (!__use_tzfile)
>     offtime = __offtime (...);
> 
>   __libc_lock_lock (tzset_lock);

I don't think it's that easy, because __use_tzfile is protected by
tzset_lock.

We could call __offtime() outside the lock unconditionally, but that
would slow down the common case where __use_tzfile is set.

I'm intending to look at deeper changes to allow the entire 
__tzfile_compute() / tz_compute() to be done outside the lock, by taking
a reference on the parsed tz data.  (I think I will probably need to sort
the copyright assignment stuff for that though).

    - Kevin

On 26 Feb 2015 02:29, Kevin Easton wrote:
> On Tue, Feb 24, 2015 at 12:01:58AM -0500, Mike Frysinger wrote:
> > On 12 Feb 2015 00:37, Kevin Easton wrote:
> > > This patch is an "easy win" partial fix for BZ #16145, which notes
> > > the heavy contention on tzset_lock when multiple threads are converting
> > > times with localtime_r().
> > > 
> > > In __tz_convert(), the lock does not need to be held after
> > > __tzfile_compute() / __tz_compute() have been called, so we can move the
> > > unlock up.  At this point there is still significant work to be done in
> > > __offtime(), so we see some improvement (in my testing with 8 cores
> > > banging on localtime_r(), ~20% improvement in throughput).
> > 
> > my reading of __offtime is that it only operates on its arguments (or const 
> > global data like __mon_yday).  it also looks expensive, so maybe we should 
> > hoist the remaining call outside of holding the lock ?  can you see if that'd 
> > have any measurable improvement ?
> > 
> >   ...
> >   int offtime = 0;
> >   if (!__use_tzfile)
> >     offtime = __offtime (...);
> > 
> >   __libc_lock_lock (tzset_lock);
> 
> I don't think it's that easy, because __use_tzfile is protected by
> tzset_lock.

blah, i was thinking it was a local.  man i hate global state :).

> We could call __offtime() outside the lock unconditionally, but that
> would slow down the common case where __use_tzfile is set.
> 
> I'm intending to look at deeper changes to allow the entire 
> __tzfile_compute() / tz_compute() to be done outside the lock, by taking
> a reference on the parsed tz data.  (I think I will probably need to sort
> the copyright assignment stuff for that though).

yes, most likely that level of work would require assignment
-mike

On 02/25/2015 11:11 AM, Mike Frysinger wrote:
> blah, i was thinking it was a local.  man i hate global state

Me too.  Is this a good time to mention the project to add to glibc 
thread-safe TZ functions, a la the reference TZ code?  They don't have 
global state.

https://sourceware.org/glibc/wiki/Development_Todo/Master#MT-Safe_and_TZ-aware_functions

On Wed, Feb 25, 2015 at 11:45:18AM -0800, Paul Eggert wrote:
> On 02/25/2015 11:11 AM, Mike Frysinger wrote:
> >blah, i was thinking it was a local.  man i hate global state
> 
> Me too.  Is this a good time to mention the project to add to glibc
> thread-safe TZ functions, a la the reference TZ code?  They don't
> have global state.
> 
> https://sourceware.org/glibc/wiki/Development_Todo/Master#MT-Safe_and_TZ-aware_functions

Yes - that should share a lot of code, I'll keep that in mind.

Are those functions on the POSIX standardisation radar?

    - Kevin

Kevin Easton wrote:
> Are those functions on the POSIX standardisation radar?

Not yet.  Their radars can't see over the horizon.