Thread local storage

Thread local storage

[Ludovic Courtès]

[-- Attachment #1: Type: text/plain, Size: 3045 bytes --]

Hello,

[Gmane swallowed the original message with ID <87hbuf8ddq.fsf@inria.fr>
so I'm resending it.  Beware of Gmane...]

I looked again at how/whether we could improve thread-local storage
access, using compiler support (the ‘__thread’ storage class).

There’s now only one thread-local datum in libguile, which is the
pointer to the current thread.  It’s accessed mostly when running
‘SCM_TICK’ (at least on every ‘call’, ‘goto/args’, or ‘return’ VM
instruction) and when dealing with dynwinds and dynamic state.

There are 4 TLS access models [0]:

  1. The ‘general-dynamic’ model (the default), involves run-time
     overhead slightly lower than ‘pthread_getspecific ()’, but
     comparable when there are few thread-local variables.

  2. The ‘local-dynamic’ model can be used when thread-local variables
     are only referenced from within the shared object they appear in.
     It is not advantageous when only one thread-local variable is used
     (per Section 4.2 of [0]), which is our case.

  3. The ‘initial-exec’ model noticeably reduces the run-time overhead.
     In addition to the restrictions in for ‘local-dynamic’, the shared
     object must be present when the executable is loaded and cannot be
     dlopened.

  4. The ‘local-exec’ model, an optimization of the above, which can
     only be used when the executable is statically linked against
     libguile and its dependencies.

So #3 is appealing (~8% speedup on ‘gcbench’, ~10% on 30 iterations of
‘nboyer’).  Unfortunately it’s not generally applicable to libguile
since libguile may be dlopened (e.g., the XChat-Guile plug-in).
(Actually, according to [1], we should be able to use it on GNU systems
since we don't use much TLS, but in practice it breaks the allocation of
libgc's TLS for some reason.  See attached test program.)

So we’re left with #1, which doesn’t buy us much performance-wise (for
‘general-dynamic’: ~3% speedup on ‘gcbench’, ~4% on 30 iterations of
‘nboyer’.)

The relevant work in ‘wip-tls’:

  commit 8346727c49c51a9668f10b507daff62dd889850a
  Author: Ludovic Courtès <ludo@gnu.org>
  Date:   Fri Oct 2 15:02:52 2009 +0200

      Deprecate `scm_mask_ints'.

  commit b9619bff4ddff267149e7e869ef3c2bcb9c4f4b4
  Author: Ludovic Courtès <ludo@gnu.org>
  Date:   Fri Oct 2 15:28:29 2009 +0200

      Arrange so that `SCM_I_CURRENT_THREAD' is not accessed outside of libguile.

  commit a24c958689c86ac520b73bc9c6e1c40cfbf6f857
  Author: Ludovic Courtès <ludo@gnu.org>
  Date:   Fri Oct 2 16:32:34 2009 +0200

      Use TLS when available for `SCM_I_CURRENT_THREAD'.

Given the second commit, changing the TLS access model for libguile
doesn’t change the ABI.  Power users can compile Guile with the TLS
model of their choice, which is nice.  ;-)

Comments?

Thanks,
Ludo’.

[0] http://people.redhat.com/drepper/tls.pdf
[1] http://thread.gmane.org/gmane.comp.lib.phil/619



[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: the dlopen(3) test program --]
[-- Type: text/x-csrc, Size: 476 bytes --]

#include <dlfcn.h>
#include <stdlib.h>

int
main (int argc, char *argv[])
{
  void *lib;
  void (*init) (void);
  void * (*eval) (const char *);

  lib = dlopen ("libguile.so", RTLD_LAZY);
  init = dlsym (lib, "scm_init_guile");
  eval = dlsym (lib, "scm_c_eval_string");

  init ();
  eval ("(format #t \"hello, world~%2 + 2 = ~A~%\" (+ 2 2))");

  return EXIT_SUCCESS;
}

/*
   Local Variables:
   compile-command: "gcc -g -Wall -o tls-dlopen tls-dlopen.c -ldl"
   End:
 */

Re: Thread local storage

[Ludovic Courtès]

ludo@gnu.org (Ludovic Courtès) writes:

>   commit b9619bff4ddff267149e7e869ef3c2bcb9c4f4b4
>   Author: Ludovic Courtès <ludo@gnu.org>
>   Date:   Fri Oct 2 15:28:29 2009 +0200
>
>       Arrange so that `SCM_I_CURRENT_THREAD' is not accessed outside of libguile.

[...]

> Given the second commit, changing the TLS access model for libguile
> doesn’t change the ABI.

AFAICS it also means that the ABI is the same for --with-threads and
--without-threads builds, unlike in 1.8.

Thanks,
Ludo’.

Re: Thread local storage

[Neil Jerram]

ludo@gnu.org (Ludovic Courtès) writes:

> I looked again at how/whether we could improve thread-local storage
> access, using compiler support (the ‘__thread’ storage class).

I worry a bit about having another build option, that might end up not
used and so bitrotting.  Also I wonder, if #1 is always better than the
current implementation of pthread_getspecific, why the implementation of
pthread_getspecific isn't rewritten so that it uses #1 inside.  Maybe
the signature of the pthread_getspecific API doesn't allow that.

3-4% is well worth having, though, and it seems (from Ken's response)
that we will definitely have to make sure that the pthread_getspecific
implementation keeps working too.  So I think this is a good
direction.

> So #3 is appealing (~8% speedup on ‘gcbench’, ~10% on 30 iterations of
> ‘nboyer’).  Unfortunately it’s not generally applicable to libguile
> since libguile may be dlopened (e.g., the XChat-Guile plug-in).

(I presume it's actually the plug-in that gets dlopened, and the plug-in
links to libguile.  I also presume that that doesn't make any difference
in practice.)

> The relevant work in ‘wip-tls’:
>
>   commit 8346727c49c51a9668f10b507daff62dd889850a
>   Author: Ludovic Courtès <ludo@gnu.org>
>   Date:   Fri Oct 2 15:02:52 2009 +0200
>
>       Deprecate `scm_mask_ints'.

Is there any replacement, or is this something that there was never any
reason to expose?  It would be good for the deprecation comment to say a
bit more to justify the deprecation.

>   commit b9619bff4ddff267149e7e869ef3c2bcb9c4f4b4
>   Author: Ludovic Courtès <ludo@gnu.org>
>   Date:   Fri Oct 2 15:28:29 2009 +0200
>
>       Arrange so that `SCM_I_CURRENT_THREAD' is not accessed outside of libguile.

This is great.  I recommend writing the NEWS soon too, so as not to
build up a backlog of such items that have been removed from the API.

>   commit a24c958689c86ac520b73bc9c6e1c40cfbf6f857
>   Author: Ludovic Courtès <ludo@gnu.org>
>   Date:   Fri Oct 2 16:32:34 2009 +0200
>
>       Use TLS when available for `SCM_I_CURRENT_THREAD'.
>
> Given the second commit, changing the TLS access model for libguile
> doesn’t change the ABI.  Power users can compile Guile with the TLS
> model of their choice, which is nice.  ;-)

+    [AC_COMPILE_IFELSE([AC_LANG_PROGRAM([static __thread int tls_integer;],
+                         [tls_integer = 123;])],

Since we don't use static for scm_i_current_thread, I guess it would be
a more accurate test not to use static here.

+  pf ("/* Define the 1 if the compiler supports the "
+      "`__thread' storage class.  */\n");

s/Define the/Define as/

Regards,
        Neil

Re: Thread local storage

[Ludovic Courtès]

Hello,

Neil Jerram <neil@ossau.uklinux.net> writes:

> ludo@gnu.org (Ludovic Courtès) writes:
>
>> I looked again at how/whether we could improve thread-local storage
>> access, using compiler support (the ‘__thread’ storage class).
>
> I worry a bit about having another build option, that might end up not
> used and so bitrotting.

Right, it increases the configuration space, and in practice most people
will end up using ‘__thread’.  OTOH I think the change is quite
isolated, since we have only one thread-local variable.

> Also I wonder, if #1 is always better than the
> current implementation of pthread_getspecific, why the implementation of
> pthread_getspecific isn't rewritten so that it uses #1 inside.  Maybe
> the signature of the pthread_getspecific API doesn't allow that.

pthread_getspecific(3) is a function, whereas ‘__thread’ requires
support from the compiler and dynamic linker.

>> So #3 is appealing (~8% speedup on ‘gcbench’, ~10% on 30 iterations of
>> ‘nboyer’).  Unfortunately it’s not generally applicable to libguile
>> since libguile may be dlopened (e.g., the XChat-Guile plug-in).
>
> (I presume it's actually the plug-in that gets dlopened, and the plug-in
> links to libguile.  I also presume that that doesn't make any difference
> in practice.)

Yes.

>> The relevant work in ‘wip-tls’:
>>
>>   commit 8346727c49c51a9668f10b507daff62dd889850a
>>   Author: Ludovic Courtès <ludo@gnu.org>
>>   Date:   Fri Oct 2 15:02:52 2009 +0200
>>
>>       Deprecate `scm_mask_ints'.
>
> Is there any replacement, or is this something that there was never any
> reason to expose?  It would be good for the deprecation comment to say a
> bit more to justify the deprecation.

I didn’t understand the point of this macro, and it’s not documented,
which is why the deprecation message doesn’t suggest any replacement.

Actually, it seems that it was used as an lvalue to mask block asyncs:

  http://google.com/codesearch?q=scm_mask_ints&hl=en&btnG=Search+Code

In that case, I don’t know how we could provide a useful compatibility
layer.

Should we worry?

>>   commit b9619bff4ddff267149e7e869ef3c2bcb9c4f4b4
>>   Author: Ludovic Courtès <ludo@gnu.org>
>>   Date:   Fri Oct 2 15:28:29 2009 +0200
>>
>>       Arrange so that `SCM_I_CURRENT_THREAD' is not accessed outside of libguile.
>
> This is great.  I recommend writing the NEWS soon too, so as not to
> build up a backlog of such items that have been removed from the API.

Well, Andy has been very good at it so far.  ;-)

Besides, ‘SCM_I_CURRENT_THREAD’ is *not* part of the API.

> +    [AC_COMPILE_IFELSE([AC_LANG_PROGRAM([static __thread int tls_integer;],
> +                         [tls_integer = 123;])],
>
> Since we don't use static for scm_i_current_thread, I guess it would be
> a more accurate test not to use static here.

Yes, why not.

> +  pf ("/* Define the 1 if the compiler supports the "
> +      "`__thread' storage class.  */\n");
>
> s/Define the/Define as/

Noted.

Thanks,
Ludo’.

Re: Thread local storage

[Neil Jerram]

ludo@gnu.org (Ludovic Courtès) writes:

> Right, it increases the configuration space, and in practice most people
> will end up using ‘__thread’.  OTOH I think the change is quite
> isolated, since we have only one thread-local variable.

Yes, I agree.

>> Also I wonder, if #1 is always better than the
>> current implementation of pthread_getspecific, why the implementation of
>> pthread_getspecific isn't rewritten so that it uses #1 inside.  Maybe
>> the signature of the pthread_getspecific API doesn't allow that.
>
> pthread_getspecific(3) is a function, whereas ‘__thread’ requires
> support from the compiler and dynamic linker.

OK.

>>>       Deprecate `scm_mask_ints'.
>>
>> Is there any replacement, or is this something that there was never any
>> reason to expose?  It would be good for the deprecation comment to say a
>> bit more to justify the deprecation.
>
> I didn’t understand the point of this macro, and it’s not documented,
> which is why the deprecation message doesn’t suggest any replacement.

Well, I was going to suggest that we could add something like this
sentence to the deprecation message.  However...

> Actually, it seems that it was used as an lvalue to mask block asyncs:
>
>   http://google.com/codesearch?q=scm_mask_ints&hl=en&btnG=Search+Code

... this search makes it clear that there's not really any point in
doing that, since no evidence of any current application Guile using
scm_mask_ints.

> In that case, I don’t know how we could provide a useful compatibility
> layer.
>
> Should we worry?

Given the above, no.

>> This is great.  I recommend writing the NEWS soon too, so as not to
>> build up a backlog of such items that have been removed from the API.
>
> Well, Andy has been very good at it so far.  ;-)
>
> Besides, ‘SCM_I_CURRENT_THREAD’ is *not* part of the API.

True, but I think I'd like the 2.0 NEWS to have an explicit entry saying
that we have cleaned up API visibility, and listing the removed names.

I have in mind to try to produce this myself before 2.0, using some
systematic comparison of the headers and libraries, but it would help if
we all start noting (i.e. in NEWS) removed names (and which we are sure
_should_ be removed) as we go along, so as to reduce the task later of
checking the removals that aren't already listed.

What do you think?

(Also note that this commit also removes SCM_STACK_OVERFLOW_P from the
API.  I guess that was never intended to be part of the API either.)

Regards,
        Neil

Re: Thread local storage

[Ludovic Courtès]

Hi,

Neil Jerram <neil@ossau.uklinux.net> writes:

> ludo@gnu.org (Ludovic Courtès) writes:

[...]

>>>>       Deprecate `scm_mask_ints'.
>>>
>>> Is there any replacement, or is this something that there was never any
>>> reason to expose?  It would be good for the deprecation comment to say a
>>> bit more to justify the deprecation.
>>
>> I didn’t understand the point of this macro, and it’s not documented,
>> which is why the deprecation message doesn’t suggest any replacement.
>
> Well, I was going to suggest that we could add something like this
> sentence to the deprecation message.  However...
>
>> Actually, it seems that it was used as an lvalue to mask block asyncs:
>>
>>   http://google.com/codesearch?q=scm_mask_ints&hl=en&btnG=Search+Code
>
> ... this search makes it clear that there's not really any point in
> doing that, since no evidence of any current application Guile using
> scm_mask_ints.

Indeed.

>> Besides, ‘SCM_I_CURRENT_THREAD’ is *not* part of the API.
>
> True, but I think I'd like the 2.0 NEWS to have an explicit entry saying
> that we have cleaned up API visibility, and listing the removed names.

For symbols, the comparison can be done quite easily with ‘objdump -T’.
For macros, it’s more work.

However, I think we should not worry too much about ‘scm_i_’ things that
were removed, with the exception of commonly used idioms such as
‘scm_i_string_chars ()’.  A detailed list of differences could be
helpful to those who have been shamelessly using internals, though.

Keeping an eye on the exported symbols is something that should be more
easily feasible from 2.0 on since internal symbols are not exported.
For internal macros, the best is to avoid them in public headers, but we
can’t avoid all of them, unless we use separate private header(s).

> (Also note that this commit also removes SCM_STACK_OVERFLOW_P from the
> API.  I guess that was never intended to be part of the API either.)

Right, that’s what I thought.

Thanks,
Ludo’.

Re: Thread local storage

[Ludovic Courtès]

Hello!

I’ve taken your remarks (Ken’s and Neil’s) into account and merged it
into ‘master’.

Thanks,
Ludo’.

Re: Thread local storage

[Ken Raeburn]

On Oct 4, 2009, at 10:03, Ludovic Courtès wrote:
> I looked again at how/whether we could improve thread-local storage
> access, using compiler support (the ‘__thread’ storage class).

For the most part I think the wip-tls changes look good.  In the non- 
__thread case, though, you've eliminated the pthread_key_create call,  
making it not work on my Mac.  (In addition to the still-present  
i18n.c compilation problem.)  Assuming that __thread variables are by  
default initialized to 0 (or NULL or whatever we specify), we also  
don't need anything in init_thread_key when we have __thread support.   
I suggest something like the patch below.  I haven't tried it on a  
system *with* the __thread support though.

Ken

--- a/libguile/threads.c
+++ b/libguile/threads.c
@@ -538,11 +538,13 @@ on_thread_exit (void *v)

  static scm_i_pthread_once_t init_thread_key_once =  
SCM_I_PTHREAD_ONCE_INIT;

+#ifndef SCM_HAVE_THREAD_STORAGE_CLASS
  static void
  init_thread_key (void)
  {
-  SET_CURRENT_THREAD (NULL);
+  scm_i_pthread_key_create (&scm_i_thread_key, NULL);
  }
+#endif

  /* Perform any initializations necessary to bring the current thread
     into guile mode, initializing Guile itself, if necessary.
@@ -561,7 +563,9 @@ scm_i_init_thread_for_guile (SCM_STACKITEM *base,  
SCM parent)
  {
    scm_i_thread *t;

+#ifndef SCM_HAVE_THREAD_STORAGE_CLASS
    scm_i_pthread_once (&init_thread_key_once, init_thread_key);
+#endif

    t = SCM_I_CURRENT_THREAD;
    if (t == NULL)

Re: Thread local storage

[Ludovic Courtès]

Hi Ken,

Ken Raeburn <raeburn@raeburn.org> writes:

> For the most part I think the wip-tls changes look good.  In the non-
> __thread case, though, you've eliminated the pthread_key_create call,
> making it not work on my Mac.

Oops, thanks for pointing it out.

(I find it surprising that MacOS doesn’t have TLS support.)

> Assuming that __thread variables are by default initialized to 0 (or
> NULL or whatever we specify), we also don't need anything in
> init_thread_key when we have __thread support.

Right.

> I suggest something like the patch below.  I haven't tried it on a
> system *with* the __thread support though.

Thanks, I’ll give it a try.

Ludo’.