What would the semantics of this be, exactly ? What does the lock protect ?

It's basically just a cheap per-object lock.  It's up to users to use it as they see fit.  For example, a library like pango can lock/unlock all fontmap accesses to provide fontmap thread-safety.

(In reply to comment #2)
> It's basically just a cheap per-object lock.  It's up to users to use it as
> they see fit.  For example, a library like pango can lock/unlock all fontmap
> accesses to provide fontmap thread-safety.

This sounds dangerous. Suppose app X starts using this generic lock on objects handed out by library Y. Later, library Y starts using the lock. Boom...

(In reply to comment #3)
> (In reply to comment #2)
> > It's basically just a cheap per-object lock.  It's up to users to use it as
> > they see fit.  For example, a library like pango can lock/unlock all fontmap
> > accesses to provide fontmap thread-safety.
> 
> This sounds dangerous. Suppose app X starts using this generic lock on objects
> handed out by library Y. Later, library Y starts using the lock. Boom...

Why would the app want to lock the object owned by the library?  Unless the library docs suggest that it's ok to do so, I don't see why.  Sure there's a thousand ways to shoot oneself in the foot...

and java and C# provide this functionality at the language syntax level, and people manage to survive

Created attachment 154571 [details] [review]
Draft patch

Here is a concept patch.  I'm not satisfied with the disposal, but it's easier than duplicating the g_object_unref() logic.  The idea is that when a locked object is unref'ed of its last reference, the object lives until it's unlocked.

Doesn't this break for instance toggle-refs?

Wouldn't it be easier for g_object_lock() to actually take a ref which is released at end of unlock() ?

(In reply to comment #7)
> Doesn't this break for instance toggle-refs?

Don't think so.  That's exactly what the final unref() handles.

> Wouldn't it be easier for g_object_lock() to actually take a ref which is
> released at end of unlock() ?

Right.  That would be essentially moving the atomic_inc from unlock() to lock().  The problem with that is that then you can't detect an unref() taking that away at the right place.

Created attachment 154622 [details] [review]
Updated patch

Minor improvements.

I don't understand how the final unref helps fix toggle refs.

Say you have an object with refcount 1. Then you lock it, and while its locked we take another ref, this does:

  old_val = g_atomic_int_exchange_and_add ((int *)&object->ref_count, 1);

  if (old_val == 1 && OBJECT_HAS_TOGGLE_REF (object))
    toggle_refs_notify (object, FALSE);

At this time object->ref_count is 0x80000001, and the add will make it 0x80000002.

However, the old_val == 1 check will fail and we will not detect that there is now multiple refs to the object and do the toggle ref callback.

Similar issues happen in the unref code:

      /* if we went from 2->1 we need to notify toggle refs if any */
      if (old_ref == 2 && has_toggle_ref) /* The last ref being held in this case is owned by the toggle_ref */
	toggle_refs_notify (object, TRUE);

The old_ref == 2 comparison will just never work out for a locked object.


Also, in various places we do things like:
  if (g_atomic_int_get (&object->ref_count) == 0)

Which all fail in the ref_count == 0 but locked case. 

This just feels risky to me. Either the lock should get a real ref on the object, or we need to mask out the flag everywhere where the refcount is read.

What exactly do you mean by "you can't detect an unref() taking
that away at the right place"?

Ryan, do you have time/interest to look into this?

I'm skeptical of the value of this over simply using g_object_{get,set}_data to hook a private mutex off an existing object.

(In reply to comment #12)
> I'm skeptical of the value of this over simply using g_object_{get,set}_data to
> hook a private mutex off an existing object.

Yeah, but that would be orders of magnitude slower...  Really, the usecase is the same as that of g_bit_lock().

(In reply to comment #13)
> (In reply to comment #12)
> > I'm skeptical of the value of this over simply using g_object_{get,set}_data to
> > hook a private mutex off an existing object.
> 
> Yeah, but that would be orders of magnitude slower...  Really, the usecase is
> the same as that of g_bit_lock().

order*s* of magnitude?  Well okay, even if it was 100 times slower, we're only talking about microseconds here.  And in any truly speed critical code (and I seriously doubt things get to this level) you can always convert things to use a global static mutex around particular sections.

(In reply to comment #5)
> and java and C# provide this functionality at the language syntax level, and
> people manage to survive

Are java and c# object locks recursive? That would make such independent use easier.

From http://tutorials.jenkov.com/java-concurrency/locks.html:

Synchronized blocks in Java are reentrant. This means, that if a Java thread enters a synchronized block of code, and thereby take the lock on the monitor object the block is synchronized on, the thread can enter other Java code blocks synchronized on the same monitor object.

(In reply to comment #16)
> From http://tutorials.jenkov.com/java-concurrency/locks.html:
> 
> Synchronized blocks in Java are reentrant. This means, that if a Java thread
> enters a synchronized block of code, and thereby take the lock on the monitor
> object the block is synchronized on, the thread can enter other Java code
> blocks synchronized on the same monitor object.

I agree that makes a huge difference in the code.  Maybe we try adding a mutex to PangoFcFontMap and see how it goes, vs a recursive-mutex.

If we want recursive-mutex, then this bug is moot I guess.

Btw, Why are we only using two bits for G_DATALIST_FLAGS_MASK?

GData is allocated with g_slice, which seem to allocate in multiples of 8 for 32bit and by 16 on 64bit, so if the main chunks are aligned there should always be 3 bits free.

Yeah, Ryan and I were talking about it a couple years ago.  Should be safe changing it to 7, or even make it dependent on the architecture.

I was under the impression that no one except for gobject really uses gdatalist.  But a Google codesearch proved otherwise.

There is the G_SLICE=always-malloc case to handle, but i think that will just work too.

Yeah I think it's safe to assume that malloc() returns (2*sizeof(void*))-aligned too.  In fact that's implied for any platform that has a 64-bit int type.

In fact, g_slice_alloc does the rounding before deciding on using malloc or not, so its safe.

I actually don't ever remember thinking that this was a great idea (I think Behdad put me up to it) ;)

fwiw, malloc() on FreeBSD often returns things that are not multiples of 8.

I think we should just forget about this.