Re: Atomic Ints (Was: GObject thread safety)

[Alex Larsson <alexl redhat com>]

On Fri, 2 Nov 2001, Sebastian Wilhelmi wrote:

> Hi,
> 
> > Otoh, we could add some atomic ops to implement refcounting that is MT
> > safe without having the overhead of locking. I've did this for the
> > ref/unref in the Mozilla javascript engine, and it makes quite a
> > difference (compared to locks).
> 
> I've for some time now wanted to include that and given, how long the GLib API
> of 2.0 will be fixed this indeed looks like a valuable addition.
> Until now I hesitated, because:
> 1) Its will add much asm-code-snippets to glib. That might prove hard to
>    maintain.
> 2) I do not know of any _L_GPL implementation of this yet. We have nspr
>    under GPL, so we would have to ask to include the asm-snippets
>    verbatim, even though I'm sure, they would allow using that.

Aren't they tripple licensing all mozilla code?

> 3) I had other things to do, that my prof deemed more important ;-)

Don't we all :)
 
> We could add special asm code for different platforms later and for the time
> being just present a sane API and make a fallback implementation and maybe a
> linux/x86 as the obviously most important one. 

Yes. The idea would be to have a fallback using normal locks.

> So I want to hereby suggest adding:
> 
> /* not opaque, but prolly in glibconfig.h */
> typedef struct _GAtomicInt GAtomicInt;
> 
> /* initialize it, either compile time or run time, just like GStatic* */
> #define G_ATOMIC_INT_INIT(init_value) ...
> void g_atomic_int_init (GAtomicInt *atomic_int, gint init_value);
> 
> /* increment by one; return new value */
> gint g_atomic_int_inc (GAtomicInt *atomic_int);
> 
> /* decrement by one; return new value */
> gint g_atomic_int_dec (GAtomicInt *atomic_int);

These are a bit more general than what is strictly needed. Are you sure 
the extra generality does not mean problems or inefficiency in the 
implementation for some arch?

We really only need 
void atomic_add (GAtomicInt *)
gboolwan atomic_dec_and_test_if_zero (GAtomicInt *)
 
> /* Reads the old value and set the new value atomically */
> gint g_atomic_int_replace (GAtomicInt *atomic_int);
> 
> /* Just relying on a write or read to an int being atomic, just to for
>    information encapsulating for this special case. Prolly just macros */
> void g_atomic_int_set (GAtomicInt *atomic_int, gint value);
> gint g_atomic_int_get (GAtomicInt *atomic_int);
> 
> /* This last one doesn't seem very necessary, but is included in 
>    libnspr. */
> gint g_atomic_int_add (GAtomicInt *atomic_int, gint to_add);
> 
> /* Free allocated resources, if any */
> void g_atomic_int_free (GAtomicInt *atomic_int);
> 
> Why not doing it like nspr, i.e.
> 
> gint g_atomic_int_inc (gint *atomic_int);
> 
> 1) The nspr fallback uses some (10 IIRC) mutexes for all atomic ints
>    (chosen by a hash value of the address), thus you can get some
>    contention (Ok. IRL this shouldn't be a problem). So the above solution
>    is more clean.
> 2) You do not by accident use the value directly. Even though of course
>    the get and set operation is atomic by nature (I would claim), it is  
>    good to set and get those vars via a function to make it clear, that
>    the values are volatile by nature.
> 
> 
> Open question: 
> 
> GAtomicInt of course is just a struct with GStaticMutex and a gint for the
> fallback case. For the optimized case we of course want it to only be a gint. 
> 
> But as we probably only support the asm-Syntax of gcc, we get a problem: The
> size of GAtomicInt depends on the compiler used for compiling glib. That
> means, that a glib compiled by gcc is not binary compatible with a glib
> compiled by some other compiler for the same platform and programs assuming so
> will fail in mysterious ways. So _that_ might be a real reason to do it like
> nspr actually. 

Yes. Having GAtomicInt *only* contain an integer of appropriate size to be 
atomic on the cpu seems like the correct choice.
 
> (BTW.: It might seem, that glib already is binary incompatibel
>  between different compilers on the same platform, but I don't think, it
>  really is, though I might be wrong)

I think the headers are source incompatible. If you build it with gcc it 
adds gccisms to glibconfig.h or something.
 
> So in retrospect I changed my mind ;-) We do i like that: (a mixture between
> nspr (implementation) and the first idea (interface))
> 
> typedef volatile gint GAtomicInt;

The kernel uses
typedef struct { volatile int counter; } atomic_t;

I think there was some reason for this. Can't remember right now.
 
> /* increment by one; return new value */
> gint g_atomic_int_inc (GAtomicInt *atomic_int);

I like the performance of the linux kernel one:
static __inline__ void atomic_inc(atomic_t *v)
{
	__asm__ __volatile__(
		LOCK "incl %0"
		:"=m" (v->counter)
		:"m" (v->counter));
}

Basically, it's just one instruction with the buslock prefix. Not having 
to return the new value (atomically) makes it a lot faster.
 
> /* decrement by one; return new value */
> gint g_atomic_int_dec (GAtomicInt *atomic_int);

Here is the kernel dec_and_test:
static __inline__ int atomic_dec_and_test(atomic_t *v)
{
	unsigned char c;

	__asm__ __volatile__(
		LOCK "decl %0; sete %1"
		:"=m" (v->counter), "=qm" (c)
		:"m" (v->counter) : "memory");
	return c != 0;
}

Also very fast.
 
> /* Reads the old value and set the new value atomically */
> gint g_atomic_int_replace (GAtomicInt *atomic_int);
> 
> /* This last one doesn't seem very necessary, but is included in 
>    libnspr. */
> gint g_atomic_int_add (GAtomicInt *atomic_int, gint to_add);

I don't think we need this.
 
> What do you think? Please choose the version and I will cook up a patch with
> just the fallback implementation so far (thus fixing the API).

I think we should try it and measure the performance implications this has 
for g_object_ref/unref(). By using very tight asm it may perform very near to 
the thread-unsafe version. If so, i think we should use those by default.

/ Alex