Message ID | 20150211133719.GA19495@chicago.guarana.org |
---|---|
State | Committed |
Headers |
Received: (qmail 25464 invoked by alias); 11 Feb 2015 12:37:49 -0000 Mailing-List: contact libc-alpha-help@sourceware.org; run by ezmlm Precedence: bulk List-Id: <libc-alpha.sourceware.org> List-Unsubscribe: <mailto:libc-alpha-unsubscribe-##L=##H@sourceware.org> List-Subscribe: <mailto:libc-alpha-subscribe@sourceware.org> List-Archive: <http://sourceware.org/ml/libc-alpha/> List-Post: <mailto:libc-alpha@sourceware.org> List-Help: <mailto:libc-alpha-help@sourceware.org>, <http://sourceware.org/ml/#faqs> Sender: libc-alpha-owner@sourceware.org Delivered-To: mailing list libc-alpha@sourceware.org Received: (qmail 25439 invoked by uid 89); 11 Feb 2015 12:37:49 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=1.7 required=5.0 tests=BAYES_05, TO_NO_BRKTS_PCNT autolearn=no version=3.3.2 X-HELO: chicago.guarana.org Date: Thu, 12 Feb 2015 00:37:19 +1100 From: Kevin Easton <kevin@guarana.org> To: libc-alpha@sourceware.org Subject: [PATCH][BZ #16145] Reduce lock contention in __tz_convert() Message-ID: <20150211133719.GA19495@chicago.guarana.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline User-Agent: Mutt/1.5.20 (2009-06-14) |
Commit Message
Kevin Easton
Feb. 11, 2015, 1:37 p.m. UTC
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.
Comments
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.
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; }