Created attachment 188879 [details] [review]
Implement pointer sized bitlocks

Ok, this patch doesn't work anymore with the new names of stuff. Will fix up.

Created attachment 188881 [details] [review]
Implement pointer sized bitlocks

New, working version. This is used for the stuff in bug 650458

*** Bug 651351 has been marked as a duplicate of this bug. ***

As a simplifying measure, I suggest we restrict pointer bitlocks to be contained in the lowest few bits of the pointer.  From a practical standpoint I'm sure nobody would try to use more than the lowest 2 or 3 bits anyway, and it also means that you don't end up with portability problems when moving your code from 64bit to 32bit machines.

As an added benefit, the implementation gets _much_ easier: on little endian, we never need to do any math at all (since the lowest bits are always at the same memory address).  Ditto any 32 bit machine (since pointer size is integer size).  All that's left is big endian 64bit, and in that case we simply add 4.

Created attachment 188890 [details] [review]
Implement pointer sized bitlocks

I think this should work (but I didn't test it).  Some notes:

  - changed the docs to say that it only works for bits 0..31

  - changed the since tags

  - used the simple logic described in comment 6

  - changed the interface from gpointer* to void* to avoid aliasing warnings.
    We should consider adding macros to make it safer.

  - use the same contention indicators since there is no harm in aliasing
    here (just a spurious WAKE call that the kernel will ignore anyway).  If
    we wanted to, we could increase the size of the array as an alternative.

  - recopy the function bodies from their int counterparts.  They are now
    almost identical code (with changes only to the types and addition of
    the int_address call).

Review of attachment 188890 [details] [review]:

::: glib/gbitlock.c
@@ +325,3 @@
+ * g_pointer_bit_lock:
+ * @address: a pointer to a pointer
+ * @lock_bit: a bit value between 0 and 31

I think the documentation needs to be a bit more explicit about the fact that @lock_bit is restricted to the lower half of the address.
Hiding that in the description of @lock_bit is not clear.

(In reply to comment #9)
> I think the documentation needs to be a bit more explicit about the fact that
> @lock_bit is restricted to the lower half of the address.
> Hiding that in the description of @lock_bit is not clear.

I agree.  I'll also add an assert to enforce it.

Everything else is OK?

(In reply to comment #10)
 
> Everything else is OK?

From my side, yes. Of course, fully supporting bit locks on pointers would be nicer, still. But this would solve the immediate need to move ahead on datalist scalability improvements.

From further discussion with Ryan, before he jumped on a plane, it seems what we really want here for optimal performance is to ensure that we use a 'lock bts' instruction. __sync_fetch_and_or does not quite give us the best code, so we may instead add a g_atomic_pointer_bit_test_and_set() function, and add an asm implementation for x86.

Awaiting a new patch from Ryan here.

I'm actually thinking that it would be more appropriate to provide hand-written ASM versions of bitlocking for x86 and amd64.  I don't want to provide those as standard atomic operations until we have a less-hacky way of supporting them.

See http://gcc.gnu.org/bugzilla/show_bug.cgi?id=49244 for my efforts to get something into GCC...

The pointer-sized bitlocks are working but they are *insanely* slow when contended (many times slower than the int-sized ones).  I don't know why.

(In reply to comment #13)
> I'm actually thinking that it would be more appropriate to provide hand-written
> ASM versions of bitlocking for x86 and amd64.  I don't want to provide those as
> standard atomic operations until we have a less-hacky way of supporting them.

Fine with me.

> See http://gcc.gnu.org/bugzilla/show_bug.cgi?id=49244 for my efforts to get
> something into GCC...
> 
> The pointer-sized bitlocks are working but they are *insanely* slow when
> contended (many times slower than the int-sized ones).  I don't know why.

Is that with g_atomic_pointer_and_or(), or with lock bts ?

With the g_atomic_pointer_and_or().  I'm working on the bts version.  We'll see how much of a difference it makes...

Created attachment 189181 [details] [review]
asm

Created attachment 189182 [details] [review]
pointers

Quite weird that the contended case should be so much slower. The only difference is really the size of the atomic operation... Maybe the pointer overlaps two cachelines (32bit in each) or something?

Or maybe its the atomic 32bit read after an atomic 64bit write?

Ryan found the silly bug:

#if defined (G_BIG_ENDIAN)

should be

#if G_BYTE_ORDER == G_BIG_ENDIAN