Created attachment 164615 [details] [review]
StaticString WIP

I'd like to point out that the original author of GStaticString is Iain Holmes.

See bug 612591 for a similar proposal.

Created attachment 164626 [details] [review]
StaticString WIP

attachment 164615 [details] [review] contains the nice, boxed version that doesn't make kittens cry.

attachment 164626 [details] [review] contains the evil version that overloads C strings, and it's approved by Cthulhu.

Review of attachment 164626 [details] [review]:

::: glib/gstaticstring.c
@@ +14,3 @@
+ * itself.
+ *
+ * Instead of duplicating it, a static string is reference counted.

Docs could really use some motivating text, "when should I use static strings vs. quarks vs. GString vs. regular char*"

Also some discussion of threading issues

@@ +69,3 @@
+}
+
+gpointer

why gpointer rather than char*?

@@ +70,3 @@
+
+gpointer
+g_static_string_nocopy_new (gchar  *contents,

is this called nocopy but it copies?

@@ +90,3 @@
+
+  memcpy (ret, contents, length);
+  STATIC_STRING (ret)->str = ret;

there is a maybe nicer way to do this, which is to put "char str[4]" at the end of the struct. See GtkTextLineSegment for a (complicated) example.

@@ +111,3 @@
+
+gpointer
+g_static_string_ref (gpointer str)

gpointer seems odd

@@ +119,3 @@
+  g_assert (STATIC_STRING (str)->str == str);
+
+  g_atomic_int_add (&(STATIC_STRING (str)->ref_count), 1);

g_atomic_int_inc

@@ +137,3 @@
+  _g_static_string_dump (str);
+
+  if (g_atomic_int_exchange_and_add (&(STATIC_STRING (str)->ref_count), -1) - 1 == 0)

g_atomic_int_dec_and_test

@@ +144,3 @@
+
+G_CONST_RETURN gchar *
+g_static_string_get_str (gconstpointer str)

if the idea of gpointer is to have to use this accessor it seems like there should be a struct type GStaticString even if that struct really just gets cast

alternatively just use char* and people just have to know it's a static string when they go to copy or free

kind of seems like you want a GStaticString separate boxed type because only that would allow bindings and code to easily know they can avoid copies and just ref. But avoiding multiple memory blocks seems like a nice optimization.

(In reply to comment #6)
> Review of attachment 164626 [details] [review]:
> 
> ::: glib/gstaticstring.c
> @@ +14,3 @@
> + * itself.
> + *
> + * Instead of duplicating it, a static string is reference counted.
> 
> Docs could really use some motivating text, "when should I use static strings
> vs. quarks vs. GString vs. regular char*"

true - as I said, it's a proof of concept.

> Also some discussion of threading issues
> 
> @@ +69,3 @@
> +}
> +
> +gpointer
> 
> why gpointer rather than char*?

yeah, it should be char*

> @@ +70,3 @@
> +
> +gpointer
> +g_static_string_nocopy_new (gchar  *contents,
> 
> is this called nocopy but it copies?

my mistake, it should not memcpy.

> @@ +90,3 @@
> +
> +  memcpy (ret, contents, length);
> +  STATIC_STRING (ret)->str = ret;
> 
> there is a maybe nicer way to do this, which is to put "char str[4]" at the end
> of the struct. See GtkTextLineSegment for a (complicated) example.
> 
> @@ +111,3 @@
> +
> +gpointer
> +g_static_string_ref (gpointer str)
> 
> gpointer seems odd
> 
> @@ +119,3 @@
> +  g_assert (STATIC_STRING (str)->str == str);
> +
> +  g_atomic_int_add (&(STATIC_STRING (str)->ref_count), 1);
> 
> g_atomic_int_inc

g_atomic_int_add() is what g_hash_table_ref() calls.

> @@ +137,3 @@
> +  _g_static_string_dump (str);
> +
> +  if (g_atomic_int_exchange_and_add (&(STATIC_STRING (str)->ref_count), -1) -
> 1 == 0)
> 
> g_atomic_int_dec_and_test

again, this is what g_hash_table_unref() does.
 
> @@ +144,3 @@
> +
> +G_CONST_RETURN gchar *
> +g_static_string_get_str (gconstpointer str)
> 
> if the idea of gpointer is to have to use this accessor it seems like there
> should be a struct type GStaticString even if that struct really just gets cast

> alternatively just use char* and people just have to know it's a static string
> when they go to copy or free

yeah, the idea is exactly to use char*.
 
> kind of seems like you want a GStaticString separate boxed type because only
> that would allow bindings and code to easily know they can avoid copies and
> just ref. But avoiding multiple memory blocks seems like a nice optimization.

yes, the idea is to add a new boxed type (like GStrv for gchar**) that wraps ref/unref.

(In reply to comment #7)

> > @@ +70,3 @@
> > +
> > +gpointer
> > +g_static_string_nocopy_new (gchar  *contents,
> > 
> > is this called nocopy but it copies?
> 
> my mistake, it should not memcpy.

actually, no: it must. so there can't be a nocopy version - or a g_static_string_from_static_string() variant.

Created attachment 164637 [details] [review]
StaticString WIP

You might want to allocate the memory for the static strings in blocks to prevent memory fragmentation. GQuark now does this

Created attachment 164641 [details] [review]
Immutable, refcounted strings

The g_static_string_* API creates immutable string objects that behave,
from a C developer perspective, exactly like C strings. Internally,
these strings are reference counted and checked for immutability. This
makes passing them inherently more efficient, by avoiding multiple
copy/free operations involving the system allocator.

Created attachment 164642 [details] [review]
Add a boxed GType for static strings

This allows having static strings in signal handlers and GObject
properties.

It appears that even though I wrote the initial code that Emmanuele posted at the top, I completely forgot about it and did it all over again although more completely this time.

https://bugzilla.gnome.org/show_bug.cgi?id=682345 without overloading the C string. Not sure which is better.

Created attachment 295516 [details] [review]
Add reference counted memory areas

A lot of our data structures are reference counted these days. It can be
useful to allow creating reference counted data without necessarily
duplicating all the reference counting machinery — a field in the
struct, reference and unreference functions, a GDestroyNotify-compatible
function for destroying the contents.

We can re-use the same overallocation and pointer offset trick we use
when allocating a GTypeInstance in gobject, and add all the reference
counting meta-information directly on top of the allocation size.

Created attachment 295517 [details] [review]
Add reference counted strings

It's useful to have a string type that is reference counted, instead of
copied and freed on demand.

Created attachment 295518 [details] [review]
gobject: Add a boxed type for string references

Pretty simple boxed wrapper using the GRef API directly.

Review of attachment 295516 [details] [review]:

::: glib/gref.c
@@ +118,3 @@
+  if (real->notify != NULL)
+    real->notify (ref);
+

Should wire up g_ref_unregister somewhere in here

(In reply to comment #17)
> Review of attachment 295516 [details] [review]:
> 
> ::: glib/gref.c
> @@ +118,3 @@
> +  if (real->notify != NULL)
> +    real->notify (ref);
> +
> 
> Should wire up g_ref_unregister somewhere in here

indeed. I'm still a bit on the fence on the whole debug thing. maybe it should be compiled in with G_ENABLE_DEBUG and enabled using G_DEBUG=ref-pointers

Created attachment 295522 [details] [review]
Add reference counted memory areas

A lot of our data structures are reference counted these days. It can be
useful to allow creating reference counted data without necessarily
duplicating all the reference counting machinery — a field in the
struct, reference and unreference functions, a GDestroyNotify-compatible
function for destroying the contents.

We can re-use the same overallocation and pointer offset trick we use
when allocating a GTypeInstance in gobject, and add all the reference
counting meta-information directly on top of the allocation size.

Created attachment 295523 [details] [review]
Add reference counted strings

It's useful to have a string type that is reference counted, instead of
copied and freed on demand.

Created attachment 295524 [details] [review]
Add refcounted strings for static strings

We don't need to copy the full string when dealing with static strings.

Created attachment 295525 [details] [review]
gobject: Add a boxed type for string references

Pretty simple boxed wrapper using the GRef API directly.

Created attachment 295529 [details] [review]
Add reference counted memory areas

A lot of our data structures are reference counted these days. It can be
useful to allow creating reference counted data without necessarily
duplicating all the reference counting machinery — a field in the
struct, reference and unreference functions, a GDestroyNotify-compatible
function for destroying the contents.

We can re-use the same overallocation and pointer offset trick we use
when allocating a GTypeInstance in gobject, and add all the reference
counting meta-information directly on top of the allocation size.

Created attachment 295530 [details] [review]
Add reference counted strings

It's useful to have a string type that is reference counted, instead of
copied and freed on demand.

Created attachment 295531 [details] [review]
Add refcounted strings for static strings

We don't need to copy the full string when dealing with static strings.

Created attachment 295532 [details] [review]
gobject: Add a boxed type for string references

Pretty simple boxed wrapper using the GRef API directly.

Comment on attachment 295522 [details] [review]
Add reference counted memory areas

>A lot of our data structures are reference counted these days.

and every single one of them uses g_*_ref()/g_*_unref(), so it's weird to have the abstractified version using functions called _acquire() and _release().

Admittedly, g_ref_ref() would be awful. Maybe call the type GRefCounted?

Comment on attachment 295531 [details] [review]
Add refcounted strings for static strings

>+char *
>+g_string_ref_new_static (const char *str)
>+{
>+  char *res = g_ref_alloc (sizeof (gpointer), NULL);
>+
>+  res = (char *) str;
>+
>+  return res;
>+}

You're allocating a GRef, leaking it, and then returning a non-GRef pointer...

(In reply to comment #27)
> (From update of attachment 295522 [details] [review])
> >A lot of our data structures are reference counted these days.
> 
> and every single one of them uses g_*_ref()/g_*_unref(), so it's weird to have
> the abstractified version using functions called _acquire() and _release().
> 
> Admittedly, g_ref_ref() would be awful. Maybe call the type GRefCounted?

g_ref_counted_ref()/_unref() does not really make it any better.

we could have ref/unref macro or inline wrappers for other types:

  #define my_struct_ref(o) g_ref_acquire(o)
  #define my_struct_unref(o) g_ref_release(o)

(In reply to comment #28)
> (From update of attachment 295531 [details] [review])
> >+char *
> >+g_string_ref_new_static (const char *str)
> >+{
> >+  char *res = g_ref_alloc (sizeof (gpointer), NULL);
> >+
> >+  res = (char *) str;
> >+
> >+  return res;
> >+}
> 
> You're allocating a GRef, leaking it, and then returning a non-GRef pointer...

no. I'm allocating a GRef with an additional pointer-sized slot at the end, and then assigning a pointer to the string to the pointer at the end of the GRef. g_ref_alloc() returns the address of the data you can start write into; the GRef data comes before that.

(In reply to comment #29)
> > >+char *
> > >+g_string_ref_new_static (const char *str)
> > >+{
> > >+  char *res = g_ref_alloc (sizeof (gpointer), NULL);
> > >+
> > >+  res = (char *) str;
> > >+
> > >+  return res;
> > >+}
> > 
> > You're allocating a GRef, leaking it, and then returning a non-GRef pointer...
> 
> no. I'm allocating a GRef with an additional pointer-sized slot at the end, and
> then assigning a pointer to the string to the pointer at the end of the GRef.

no, you're not :). That would be 

    -  res = (char *) str;
    +  *res = (char *) str;

but it still won't work, since now you'd be returning a char** rather than a char*. I can't see any way to make this function work (other than by making it identical to g_string_ref_new()).

Created attachment 295540 [details] [review]
Add reference counted memory areas

A lot of our data structures are reference counted these days. It can be
useful to allow creating reference counted data without necessarily
duplicating all the reference counting machinery — a field in the
struct, reference and unreference functions, a GDestroyNotify-compatible
function for destroying the contents.

We can re-use the same overallocation and pointer offset trick we use
when allocating a GTypeInstance in gobject, and add all the reference
counting meta-information directly on top of the allocation size.

Created attachment 295541 [details] [review]
Add reference counted strings

It's useful to have a string type that is reference counted, instead of
copied and freed on demand.

Created attachment 295542 [details] [review]
gobject: Add a boxed type for string references

Pretty simple boxed wrapper using the GRef API directly.

Created attachment 295543 [details] [review]
tests: Add units for the g_ref_* API

(In reply to comment #30)

> no, you're not :).

you're absolutely right, and I was evidently on crack when I wrote that.

yes, the only way to handle static strings is to memcpy() them anyway. I just dropped the patch from the set, and swapped it with a test suite.

Created attachment 295544 [details] [review]
datetime: Use GRef internally

Drop the ref_count field, and implement alloc/ref/unref with the
corresponding GRef API.

Created attachment 295545 [details] [review]
keyfile: Use GRef

Drop the internal ref_count field, and implement new/ref/unref with the
corresponsing GRef API.

two simple patches to port a couple of GLib data types to GRef, to show off the API. the test suite passes as expected.

Created attachment 295570 [details] [review]
Add reference counted memory areas

A lot of our data structures are reference counted these days. It can be
useful to allow creating reference counted data without necessarily
duplicating all the reference counting machinery — a field in the
struct, reference and unreference functions, a GDestroyNotify-compatible
function for destroying the contents.

We can re-use the same overallocation and pointer offset trick we use
when allocating a GTypeInstance in gobject, and add all the reference
counting meta-information directly on top of the allocation size.

Created attachment 295571 [details] [review]
Add reference counted strings

It's useful to have a string type that is reference counted, instead of
copied and freed on demand.

Created attachment 295572 [details] [review]
gobject: Add a boxed type for string references

Pretty simple boxed wrapper using the GRef API directly.

Created attachment 295573 [details] [review]
ref: Add new/new0 macros

Similar to g_new/g_new0.

Created attachment 295574 [details] [review]
gref: Add realloc() and dup()

Some more useful API.

Created attachment 295575 [details] [review]
gref: Remove the atomic bit

We can use the sign of the refcount field itself to know if the refcount
is supposed to be atomic or not. Positive numbers mean simple refcount,
negative numbers mean atomic refcount.

Created attachment 295576 [details] [review]
gref: Allow querying whether refcounting is atomic

It's useful, since we have a make_atomic() as well.

Created attachment 295577 [details] [review]
tests: Add units for the g_ref_* API

Created attachment 295578 [details] [review]
docs: Add GRef to the API reference

Summary of our chatting at the hackfest:

 - we should probably not use this for GLib pseudoclasses that already have
   their own tested and working refcounting that works without memory tricks

 - the name is not great.  I think GRefStruct is the best we could think up
   at the time, despite the fact that refstrings would be implemented using
   this (as a probable sole exception)

 - it should be made very clear that the _acquire() and _release() functions
   should absolutely never be called by the "user" but only by the
   implementation, which will provide its own _ref() and _unref() functions
   or macros

 - big-hammer _destroy() is very odd in a refcounted regime

 - i'd have thought it be called "ref string"

Comment on attachment 295574 [details] [review]
gref: Add realloc() and dup()

>+ * g_ref_realloc:

That can't work; other places holding a ref on it are still going to have the old pointer (and if you know all of the places that are holding a ref on it so that you can tell them the new pointer, then you don't actually need reference counting).

Related: ref strings should probably be "const char *" rather than "char *" (cf GBytes vs GByteArray; people who want a mutable string/buffer can use GString rather than a ref string).


>+ * g_ref_dup:
>+ * @ref: a reference counted memory area

>+gpointer
>+g_ref_dup (gconstpointer  data,
>+           gsize          alloc_size,
>+           GDestroyNotify notify)

(doc args don't match the code)

Created attachment 295734 [details] [review]
Add reference counted memory areas

A lot of our data structures are reference counted these days. It can be
useful to allow creating reference counted data without necessarily
duplicating all the reference counting machinery — a field in the
struct, reference and unreference functions, a GDestroyNotify-compatible
function for destroying the contents.

We can re-use the same overallocation and pointer offset trick we use
when allocating a GTypeInstance in gobject, and add all the reference
counting meta-information directly on top of the allocation size.

Created attachment 295735 [details] [review]
Add reference counted strings

It's useful to have a string type that is reference counted, instead of
copied and freed on demand.

Created attachment 295736 [details] [review]
gobject: Add a boxed type for string references

Pretty simple boxed wrapper using the GRef API directly.

Created attachment 295737 [details] [review]
tests: Add units for the g_ref_* API

Created attachment 295738 [details] [review]
docs: Add GRef to the API reference

(In reply to comment #49)
> (From update of attachment 295574 [details] [review])
> >+ * g_ref_realloc:
> 
> That can't work; other places holding a ref on it are still going to have the
> old pointer (and if you know all of the places that are holding a ref on it so
> that you can tell them the new pointer, then you don't actually need reference
> counting).

yeah, this was mostly an experiment. I just removed it.

> Related: ref strings should probably be "const char *" rather than "char *" (cf
> GBytes vs GByteArray; people who want a mutable string/buffer can use GString
> rather than a ref string).

using const would require casting everything before acquiring and releasing a reference, though. pretty ugly.

(In reply to comment #48)
> Summary of our chatting at the hackfest:
> 
>  - we should probably not use this for GLib pseudoclasses that already have
>    their own tested and working refcounting that works without memory tricks

already dropped from my patchset.

>  - the name is not great.  I think GRefStruct is the best we could think up
>    at the time, despite the fact that refstrings would be implemented using
>    this (as a probable sole exception)

considering the prior art in Glibmm, I'd go for GRefPointer more than GRefStruct.

>  - it should be made very clear that the _acquire() and _release() functions
>    should absolutely never be called by the "user" but only by the
>    implementation, which will provide its own _ref() and _unref() functions
>    or macros

noted in the docs.

>  - big-hammer _destroy() is very odd in a refcounted regime

yeah, I removed it.

>  - i'd have thought it be called "ref string"

looking again at prior art, "string ref" seems to be more used for "refcounted strings" than "ref string". examples: LLVM; the initial C++1x Google proposal for a refcounted string; the StringReference class in the Windows platform. there does not appear to be a widely used "RefString" class.

after a quick discussion at the hackfest in response to a request from Bastien, I also made the "reference counter" API public.

Created attachment 295741 [details] [review]
Add reference counted memory areas

A lot of our data structures are reference counted these days. It can be
useful to allow creating reference counted data without necessarily
duplicating all the reference counting machinery — a field in the
struct, reference and unreference functions, a GDestroyNotify-compatible
function for destroying the contents.

We can re-use the same overallocation and pointer offset trick we use
when allocating a GTypeInstance in gobject, and add all the reference
counting meta-information directly on top of the allocation size.

Created attachment 295742 [details] [review]
Add reference counted strings

It's useful to have a string type that is reference counted, instead of
copied and freed on demand.

Created attachment 295743 [details] [review]
gobject: Add a boxed type for string references

Pretty simple boxed wrapper using the GRef API directly.

Created attachment 295744 [details] [review]
tests: Add units for the g_ref_* API

Created attachment 295745 [details] [review]
docs: Add GRef to the API reference

(In reply to comment #55)
> >  - i'd have thought it be called "ref string"
> 
> looking again at prior art, "string ref" seems to be more used for "refcounted
> strings" than "ref string".

One thing that's nice about "ref string" is that then you don't have to worry about namespace clashes with GString. (Eg, if someone adds refcounting to GString later, then we'd have g_string_ref(), which would not be a string ref function.)

> > Related: ref strings should probably be "const char *" rather than "char *" (cf
> > GBytes vs GByteArray; people who want a mutable string/buffer can use GString
> > rather than a ref string).
> 
> using const would require casting everything before acquiring and releasing a
> reference, though. pretty ugly.

ah, yeah. well, if you did the "_ref/_unref macro wrappers for _acquire/_release" thing, then you'd only need the casts in the wrappers.

Review of attachment 295742 [details] [review]:

::: glib/gref.c
@@ +463,3 @@
+ * be freed.
+ *
+ * Returns: a reference counted string

Could we make it slightly more obvious that the retval is *not* the same pointer as @str? There are various other string functions which do pass through the pointer.

@@ +471,3 @@
+{
+  gsize len = strlen (str);
+  char *res = g_ref_pointer_alloc (len + 1, NULL);

Maybe there should be a
g_return_val_if_fail (str != NULL, NULL)
in here?

Wondering if the documentation should explicitly mention that it doesn’t pass NULLs through in the same way as g_strdup().

Review of attachment 295744 [details] [review]:

::: glib/tests/refcounts.c
@@ +1,2 @@
+#include <glib.h>
+#include <string.h>

Missing a copyright header.

@@ +50,3 @@
+  g_assert_true (strcmp (orig, new) == 0);
+
+  g_free (orig);

Might it make sense to overwrite @orig a bit here, so the test can’t be confounded by @orig not getting reallocated and overwritten by other code.

(In reply to comment #62)
> Review of attachment 295742 [details] [review]:
> 
> ::: glib/gref.c
> @@ +463,3 @@
> + * be freed.
> + *
> + * Returns: a reference counted string
> 
> Could we make it slightly more obvious that the retval is *not* the same
> pointer as @str? There are various other string functions which do pass through
> the pointer.

do you have any specific suggestion on the wording on the documentation? I can't really think of anything. :-/

> @@ +471,3 @@
> +{
> +  gsize len = strlen (str);
> +  char *res = g_ref_pointer_alloc (len + 1, NULL);
> 
> Maybe there should be a
> g_return_val_if_fail (str != NULL, NULL)
> in here?

indeed.
 
> Wondering if the documentation should explicitly mention that it doesn’t pass
> NULLs through in the same way as g_strdup().

refcounting NULL (or an empty string) would be a bit pointless.

(In reply to comment #63)
> Review of attachment 295744 [details] [review]:
> 
> ::: glib/tests/refcounts.c
> @@ +1,2 @@
> +#include <glib.h>
> +#include <string.h>
> 
> Missing a copyright header.

we don't usually add copyright or licensing to test cases. at least, I never did.
 
> @@ +50,3 @@
> +  g_assert_true (strcmp (orig, new) == 0);
> +
> +  g_free (orig);
> 
> Might it make sense to overwrite @orig a bit here, so the test can’t be
> confounded by @orig not getting reallocated and overwritten by other code.

good suggestion

(In reply to comment #61)
> (In reply to comment #55)
> > >  - i'd have thought it be called "ref string"
> > 
> > looking again at prior art, "string ref" seems to be more used for "refcounted
> > strings" than "ref string".
> 
> One thing that's nice about "ref string" is that then you don't have to worry
> about namespace clashes with GString. (Eg, if someone adds refcounting to
> GString later, then we'd have g_string_ref(), which would not be a string ref
> function.)

g_ref_string_new(), g_ref_string_ref(), g_ref_string_unref().

dunno, I'm not going to argue on the naming, really.

there is a remaining question as to whether we should intern the strings and/or the pointers unconditionally.
 
> > > Related: ref strings should probably be "const char *" rather than "char *" (cf
> > > GBytes vs GByteArray; people who want a mutable string/buffer can use GString
> > > rather than a ref string).
> > 
> > using const would require casting everything before acquiring and releasing a
> > reference, though. pretty ugly.
> 
> ah, yeah. well, if you did the "_ref/_unref macro wrappers for
> _acquire/_release" thing, then you'd only need the casts in the wrappers.

true.

(In reply to comment #64)
> (In reply to comment #62)
> > Review of attachment 295742 [details] [review] [details]:
> > 
> > ::: glib/gref.c
> > @@ +463,3 @@
> > + * be freed.
> > + *
> > + * Returns: a reference counted string
> > 
> > Could we make it slightly more obvious that the retval is *not* the same
> > pointer as @str? There are various other string functions which do pass through
> > the pointer.
> 
> do you have any specific suggestion on the wording on the documentation? I
> can't really think of anything. :-/

“a newly allocated reference counted string”?

> > Wondering if the documentation should explicitly mention that it doesn’t pass
> > NULLs through in the same way as g_strdup().
> 
> refcounting NULL (or an empty string) would be a bit pointless.

Yeah, and I’m sort of on the fence about it being too explicit. Maybe we could go with a (not nullable) annotation (bug #719966)?

> (In reply to comment #63)
> > Review of attachment 295744 [details] [review] [details]:
> > 
> > ::: glib/tests/refcounts.c
> > @@ +1,2 @@
> > +#include <glib.h>
> > +#include <string.h>
> > 
> > Missing a copyright header.
> 
> we don't usually add copyright or licensing to test cases. at least, I never
> did.

So how are they licenced? Is Debian OK with this? They‘re normally pretty militant about precise copyright headers.

Created attachment 295759 [details] [review]
Add reference counted memory areas

A lot of our data structures are reference counted these days. It can be
useful to allow creating reference counted data without necessarily
duplicating all the reference counting machinery — a field in the
struct, reference and unreference functions, a GDestroyNotify-compatible
function for destroying the contents.

We can re-use the same overallocation and pointer offset trick we use
when allocating a GTypeInstance in gobject, and add all the reference
counting meta-information directly on top of the allocation size.

Created attachment 295760 [details] [review]
Add reference counted strings

It's useful to have a string type that is reference counted, instead of
copied and freed on demand.

Created attachment 295761 [details] [review]
gobject: Add a boxed type for string references

Pretty simple boxed wrapper using the GRef API directly.

Created attachment 295762 [details] [review]
tests: Add units for the g_ref_* API

Created attachment 295763 [details] [review]
docs: Add GRef to the API reference

Review of attachment 295760 [details] [review]:

::: glib/gref.c
@@ +487,3 @@
+  char *res;
+
+  g_return_val_if_fail (str != NULL || *str != '\0', NULL);

This will fail iff (str == NULL && *str == '\0') which is impossible and will crash.

Explicitly disallowing the empty string seems dubious anyway — you then can’t unconditionally pass a non-NULL string to this function. If you want to keep this check, it should be (fixed and) documented, but I think it’s counterintuitive.

@@ +516,3 @@
+g_string_ref (const char *str)
+{
+  char *ref = (char *) str;

g_return_val_if_fail() missing here.

@@ +532,3 @@
+g_string_unref (const char *str)
+{
+  char *ref = (char *) str;

g_return_val_if_fail() missing here.

Review of attachment 295762 [details] [review]:

::: glib/tests/refcounts.c
@@ +51,3 @@
+  g_assert_true (strcmp (orig, new) == 0);
+
+  g_clear_pointer (&orig, g_free);

I meant ‘overwrite @orig’ in the sense of ‘memset (orig, 'a', strlen (orig)); g_free (orig);’ to test that @orig and @new are not aliases.

Review of attachment 295759 [details] [review]:

::: glib/gref.c
@@ +27,3 @@
+ * Reference counted memory areas are kept alive as long as something holds
+ * a reference on them; as soon as their reference count drops to zero, the
+ * associated memory is freed.

It would be good to add a warning that the reference counts do not default to being thread safe. Or better still make them thread safe by default.

@@ +56,3 @@
+                  gboolean      atomic)
+{
+  *refcount = atomic ? -1 : 1;

As far as I can see atomic is never true

@@ +97,3 @@
+  if (refs == -1 || refs == 1)
+    return TRUE;
+  if (refs > 0)

Is there a demand for non-atomic ref counts? The only reason to use them is if the structure is only ever accessed from one thread at a time and you are worried about performance in which case this extra branch and the g_atomic_int_get() are potentially bad (I'm not sure how much of a concern they are in practice). One possibility is to have separate atomic/non-atomic acquire and release functions and let developers choose which one's they want to use in their ref/unref functions (though you'd need different boxed types for the atomic/non-atomic variants).

@@ +272,3 @@
+
+      if (clear)
+        allocated = g_malloc0 (private_size + alloc_size + sizeof (gpointer));

If a GRef allocation cannot be resized, would it be better to use the slice allocator?

@@ +398,3 @@
+g_ref_pointer_acquire (gpointer ref)
+{
+  GRefPtr *real = G_REF_PTR (ref);

The alloc functions return a pointer past the end of the GRef structure, so don't you need to subtract that offset from ref to get a pointer to the real structure here?

@@ +428,3 @@
+g_ref_pointer_release (gpointer ref)
+{
+  GRefPtr *real = G_REF_PTR (ref);

Again don't you need to subtract an offset from ref?

Review of attachment 295760 [details] [review]:

Would it be better to use atomic ref-counts for this?

(In reply to comment #73)
> Review of attachment 295759 [details] [review]:
> 
> ::: glib/gref.c
> @@ +398,3 @@
> +g_ref_pointer_acquire (gpointer ref)
> +{
> +  GRefPtr *real = G_REF_PTR (ref);
> 
> The alloc functions return a pointer past the end of the GRef structure, so
> don't you need to subtract that offset from ref to get a pointer to the real
> structure here?

Sorry ignore that, G_REF_PTR takes care of the offset. Though is the offset the same when running under valgrind?
 
> @@ +428,3 @@
> +g_ref_pointer_release (gpointer ref)
> +{
> +  GRefPtr *real = G_REF_PTR (ref);
> 
> Again don't you need to subtract an offset from ref?
Same mistake, sorry

Review of attachment 295761 [details] [review]:

::: gobject/gboxed.c
@@ +195,3 @@
+        g_boxed_type_register_static (g_intern_static_string ("GStringRef"),
+                                      (GBoxedCopyFunc) g_ref_acquire,
+                                      (GBoxedFreeFunc) g_ref_release);

This doesn't compile, it should be g_ref_pointer_acquire and g_ref_pointer_release

Review of attachment 295763 [details] [review]:

Could this patch be merged into the one which introduces the symbols?

Also, it might be an idea to add a link to it from the g_strdup() documentation and encourage people to use it in situations where they would otherwise g_strdup() like crazy.

(In reply to comment #76)
> Review of attachment 295761 [details] [review]:
> 
> ::: gobject/gboxed.c
> @@ +195,3 @@
> +        g_boxed_type_register_static (g_intern_static_string ("GStringRef"),
> +                                      (GBoxedCopyFunc) g_ref_acquire,
> +                                      (GBoxedFreeFunc) g_ref_release);
> 
> This doesn't compile, it should be g_ref_pointer_acquire and
> g_ref_pointer_release

yes, I already fixed it locally, but didn't want to resubmit the whole patchset.

(In reply to comment #77)
> Review of attachment 295763 [details] [review]:
> 
> Could this patch be merged into the one which introduces the symbols?

sure.

> Also, it might be an idea to add a link to it from the g_strdup() documentation
> and encourage people to use it in situations where they would otherwise
> g_strdup() like crazy.

good point.

(In reply to comment #73)
> Review of attachment 295759 [details] [review]:
> 
> ::: glib/gref.c
> @@ +27,3 @@
> + * Reference counted memory areas are kept alive as long as something holds
> + * a reference on them; as soon as their reference count drops to zero, the
> + * associated memory is freed.
> 
> It would be good to add a warning that the reference counts do not default to
> being thread safe. Or better still make them thread safe by default.

I don't agree that the refcounting should be thread safe by default.

in many cases in our platform, atomic refcounting is pointless if not actively harmful for performance (atomic operations are not cheap). if you need atomic reference counting semantics, it's easy to make the reference count atomic; doing the opposite (go from atomic to non-atomic) is not safe.

the current design allows you to easily switch from the default non-atomic reference counting semantics to atomic ones with a simple call. I even removed the g_ref_pointer_alloc() wrapper that called g_ref_pointer_make_atomic() for you because it was mostly pointless — but it can be introduced back, maybe with an inlined wrapper.

> @@ +56,3 @@
> +                  gboolean      atomic)
> +{
> +  *refcount = atomic ? -1 : 1;
> 
> As far as I can see atomic is never true

not internally, no. if you want atomic reference counting semantics, you can do:

  YourType *
  your_type_new (void)
  {
    YourType *res = g_ref_pointer_new (YourType, your_type_free);

    g_ref_pointer_make_atomic (res);

    return res;
  }

as I said above, I can add back a wrapper that does it for you, but I'm wary about the combinatorial explosion of atomic_alloc, atomic_alloc0, atomic_new, and atomic_new0.

> @@ +97,3 @@
> +  if (refs == -1 || refs == 1)
> +    return TRUE;
> +  if (refs > 0)
> 
> Is there a demand for non-atomic ref counts?

yes. for some stuff we really jumped the gun with all-atomic refcounting, and it's getting worse because other libraries are copying the same pattern.

that's also one of the reasons why I added the g_ref_count_* API.

> The only reason to use them is if
> the structure is only ever accessed from one thread at a time and you are
> worried about performance in which case this extra branch and the
> g_atomic_int_get() are potentially bad (I'm not sure how much of a concern they
> are in practice).

the average cost of a g_atomic_int_get() is smaller than the cost of a g_atomic_int_inc() and g_atomic_int_dec_and_test(). I'm also being overzealous with the g_atomic_int_get(), to be fair; testing for the sign is not going to be problematic in all cases.

> One possibility is to have separate atomic/non-atomic acquire
> and release functions and let developers choose which one's they want to use in
> their ref/unref functions (though you'd need different boxed types for the
> atomic/non-atomic variants).

I'd really try to avoid a combinatorial explosion of entry points.

> @@ +272,3 @@
> +
> +      if (clear)
> +        allocated = g_malloc0 (private_size + alloc_size + sizeof (gpointer));
> 
> If a GRef allocation cannot be resized, would it be better to use the slice
> allocator?

I think we also want to reduce the use of the slice allocator, unless we want to alias g_slice_* to g_malloc on every platform except Windows.

Created attachment 296370 [details] [review]
Add reference counted memory areas

A lot of our data structures are reference counted these days. It can be
useful to allow creating reference counted data without necessarily
duplicating all the reference counting machinery — a field in the
struct, reference and unreference functions, a GDestroyNotify-compatible
function for destroying the contents.

We can re-use the same overallocation and pointer offset trick we use
when allocating a GTypeInstance in gobject, and add all the reference
counting meta-information directly on top of the allocation size.

Created attachment 296371 [details] [review]
Add reference counted strings

It's useful to have a string type that is reference counted, instead of
copied and freed on demand.

Created attachment 296372 [details] [review]
gobject: Add a boxed type for string references

Pretty simple boxed wrapper using the GRef API directly.

Created attachment 296373 [details] [review]
tests: Add units for the g_ref_* API

Created attachment 296374 [details] [review]
docs: Mention g_string_ref_new() in g_strdup()

Instead of duplicating strings, using the reference counted version may
be more efficient.

pushed the latest version of the patches to the wip/refptr branch of glib.

I'd love for this to get in 2.44, so I can start using it in a couple of projects.

I also have a couple of patches that move some of the ad hoc refcounting to the g_ref_count_* API of attachment 296370 [details] [review].

Review of attachment 296370 [details] [review]:

Looking good, but still lots of nit picking from me.  Sorry about that :/

I'd split the refcounting helpers into a separate commit (and maybe even a separate file).  There also needs to be a longer introduction to them somewhere.  A separate section header would work nicely for that, but it could also be thrown into _init() or something.

I'm still stuck on the names.  My preferred choices for names would be 'ref struct' and 'ref string'.  I don't like ref pointer because this thing is not a pointer in any way.  You can have a pointer _to_ it, but it is not a pointer itself.  Maybe it contains a pointer -- probably several, though.  I think 'struct' is probably just about the most general name we can give it.  Of course that excludes that we could be storing other things there -- including its very first use for strings.  Unless we can find something less ugly than RefMemArea, I'm afraid 'struct' is our most reasonably approximation.  I'm completely unswayed by the glibmm prior art -- a bad name is a bad name.

I'd also name the file accordingly to this (which would make more sense if we split the refcounting API out to grefcount.h or similar, or even gutils.h).

The appeal to prior art for refstrings (or 'string refs') is more convincing, I think (on account of pulling from a wider variety of sources), but it still doesn't make sense to me.  Examining some of the sources you site (LLVM for example) show that they are, in fact, references to strings:

"""
StringRef - Represent a constant reference to a string, i.e. a character array and a length, which need not be null terminated.

This class does not own the string data, it is expected to be used in situations where the character data resides in some other buffer, whose lifetime extends past that of the StringRef. For this reason, it is not in general safe to store a StringRef. 
"""

which is almost the opposite of what we have here.

::: glib/gref.c
@@ +44,3 @@
+
+/**
+ * g_ref_count_init:

on its surface this seems slightly spurious, but I think I actually like how this will look when you end up using it...

can we do an atomic set for the atomic case?

@@ +94,3 @@
+
+test_again:
+  refs = g_atomic_int_get (refcount);

hitting an atomic get right at the start is missing the point a bit here.

would be better to do an unsafe access first.  in the non-atomic case, great.  in the atomic case, you are still protected by the safe compare-and-exchange below.  of course, this probably means that we should still bother to do the compare/exchange even in the case of the last ref... i guess i could imagine some utterly weird architecture that lets us see a stale "1" value here even though another CPU bumped it up to 2 in the meantime -- would be better to make sure we are actually transitioning it from 1 to 0 before returning TRUE.

could probably also better-optimise the presumed-likely case of non-atomic/non-final ref:

if (refs > 1)
  {
    refs--;
  }
else
  {
    ... check which other case it is
  }

also: volatile could possibly hurt us here in case of compiler output and it doesn't give us any extra advantages.  even in the case of atomic accesses, we aren't really meeting "volatile" expectations which i would guess to be single read and single write...  far better to document exactly what we guarantee in the API docs.

@@ +117,3 @@
+g_ref_count_make_atomic (volatile int *refcount)
+{
+  int refs = g_atomic_int_get (refcount);

this function is a particularly odd mix of atomic and non-atomic accesses.  i'd have rather assumed that it would be the store that's atomic here, as the first act in the "new life" of the variable.

@@ +267,3 @@
+  g_return_val_if_fail (alloc_size != 0, NULL);
+
+  if (RUNNING_ON_VALGRIND)

probably deserves a comment with at least a pointer to the similar inanity in gtype.c

@@ +356,3 @@
+
+/**
+ * g_ref_pointer_take:

This is not what "take" means.

Normally I'd expect 'dup' except that this is not a straight-up dup operation either.

_new_from() ?

New attempt, in the wip/ebassi/rc branch.

I've mostly tried to reflect Allison's review in comment 87.

Patch review would be easier if the patches were on Bugzilla. Anyway, some comments:

Add reference counter API
---

 • SECTION:refcount is missing introductory documentation and a ‘Since:’ line. The introductory documentation should make it very clear that g_ref_counter_make_atomic() must only be called before an object’s refcount has been exposed to other code (for example, in the object’s constructor), otherwise there’s the possibility of a race between a non-atomic acquire() call from some code outside the object, and the make_atomic() call.
 • Should the g_ref_counter_*() API be marked as (skip)?
 • g_ref_counter_acquire(): the example in the documentation doesn’t return anything (sorry, nitpicking).
 • Nitpick: instead of labels and gotos, it looks like the ref-count-checking loops could easily be implemented as do…while loops, which would be a bit neater.
 • General comment about grefcount.c: nowhere does it actually document that the internal representation uses positive ref-counts for non-atomic accesses and negative ones for atomic accesses.
 • In g_ref_counter_release(), shouldn’t there be an atomic access in the `if (refs == -1) return TRUE` fast path in order to guarantee ordering with any competing g_ref_counter_acquire() call, as Allison mentioned in comment #87.
 • It would be useful to add opportunistic guards for functions which assert that (ref_count != 0).
 • g_ref_counter_make_atomic() should either assert that the ref-counter is currently non-atomic, or should document that it’s safe to be called multiple times (and is idempotent after the first call).
 • Do we want to add assertions to check for ref-count overflow? Previously when using positive integers to represent reference counts, overflowing from MAXINT to MININT has obviously corrupted the ref-counter to be negative. With double-ended ref-counts, overflowing will now cause the ref-count to continually atomically and non-atomically flip between MAXINT and MININT, which might be less obvious. If performance is a consideration, such checks could be disabled with G_DISABLE_CHECKS.
 • Tests are missing.

Porting commits
---

These all look good.

Add reference counted memory
---

 • Documentation and introduction section are missing.
 • Probably all need (skip) annotations.
 • g_ref_dup() is missing a precondition assertion for (data != NULL).
 • In g_ref_alloc_internal() the precondition assertion on size should also check that (alloc_size <= G_MAXSIZE - G_REF_SIZE - ALIGN_STRUCT (1) - sizeof (gpointer)), i.e. that the worst-case arithmetic won’t overflow.
 • The purpose of all the ALIGN_STRUCT(1) stuff isn’t clear to me. It seems to be a 16-byte block of space which is never used, and whose size doesn’t relate to the GRef structure size, so I’m not sure how it can be providing alignment padding.
 • In g_ref_alloc_internal(), the VALGRIND_MALLOCLIKE_BLOCK() call for the alloc_size block should set its fourth parameter to `clear` rather than `TRUE`, right?
 • Would it be useful to add VALGRIND_FREELIKE_BLOCK() calls to g_ref_free()?
 • Tests are missing.
 • I don’t think we need g_autoptr support for GRef, since it’s meant to be used with plain old data types, so presumably anyone using it should also add `G_DEFINE_AUTOPTR_CLEANUP_FUNC (MyPlainOldDataType, g_ref_release)` to their code. This should be mentioned in the documentation.

Add atomically reference counted memory
---

 • g_atomic_ref_dup() is missing a precondition assertion for (data != NULL).
 • Documentation missing.
 • Tests are missing.
 • Similarly to above, the documentation should mention that users should call `G_DEFINE_AUTOPTR_CLEANUP_FUNC (MyPlainOldAtomicDataType, g_atomic_ref_release)` for g_autoptr support.

Add reference counted strings
---

 • Documentation and introduction section are missing.
 • Probably all need (skip) annotations.
 • g_ref_string_new(): Why disallow the case of the empty string?
 • Need an overflow check for (len + 1).
 • Can we have a g_ref_string_new_len() which takes a string length to use, so we can easily extract substrings from other strings when creating ref-counted strings?
 • Tests are missing.
 • We won’t be able to use ref-counted strings with g_autofree any more, and similarly won’t be able to use them with g_autoptr or g_auto due to them not having a separate type name. We could either introduce a new g_autostr macro to allow them to be freed automatically; or could add a `typedef gchar GRefString` and `G_DEFINE_AUTOPTR_CLEANUP_FUNC (GRefString, g_ref_string_unref)`.

(In reply to Philip Withnall from comment #89)
> Patch review would be easier if the patches were on Bugzilla.

I know, but this bug is getting unwieldy already…

> Anyway, some comments:
 
Thanks!

I'm just going to reply to some of the issues; I agree with what I left out.

> Add reference counter API
> ---
> 
>  • SECTION:refcount is missing introductory documentation and a ‘Since:’
> line. The introductory documentation should make it very clear that
> g_ref_counter_make_atomic() must only be called before an object’s refcount
> has been exposed to other code (for example, in the object’s constructor),
> otherwise there’s the possibility of a race between a non-atomic acquire()
> call from some code outside the object, and the make_atomic() call.

The documentation part of the branch is completely up in the air, mostly because I didn't want to commit to it before getting a general review. Same goes for the tests.

>  • Should the g_ref_counter_*() API be marked as (skip)?

Dunno; do we foresee some language other than C using integers as reference counters?

>  • Nitpick: instead of labels and gotos, it looks like the
> ref-count-checking loops could easily be implemented as do…while loops,
> which would be a bit neater.

I've followed what existing code in GLib/GObject does. :-)

But yes: replacing with a do { … } while() is probably more readable.

>  • General comment about grefcount.c: nowhere does it actually document that
> the internal representation uses positive ref-counts for non-atomic accesses
> and negative ones for atomic accesses.

Should it be explicitly described? Do we want to commit to this strategy?

>  • In g_ref_counter_release(), shouldn’t there be an atomic access in the
> `if (refs == -1) return TRUE` fast path in order to guarantee ordering with
> any competing g_ref_counter_acquire() call, as Allison mentioned in comment
> #87.

Yes, I missed that.

>  • It would be useful to add opportunistic guards for functions which assert
> that (ref_count != 0).

Good point, though I'd keep those assertions under a G_DISABLE_CHECKS conditional.

>  • Do we want to add assertions to check for ref-count overflow? Previously
> when using positive integers to represent reference counts, overflowing from
> MAXINT to MININT has obviously corrupted the ref-counter to be negative.
> With double-ended ref-counts, overflowing will now cause the ref-count to
> continually atomically and non-atomically flip between MAXINT and MININT,
> which might be less obvious. If performance is a consideration, such checks
> could be disabled with G_DISABLE_CHECKS.

Sounds like a good thing to check.

> These all look good.
> 
> Add reference counted memory
> ---

>  • The purpose of all the ALIGN_STRUCT(1) stuff isn’t clear to me. It seems
> to be a 16-byte block of space which is never used, and whose size doesn’t
> relate to the GRef structure size, so I’m not sure how it can be providing
> alignment padding.

Again, I've lifted this code from what we do in GTypeInstance: https://git.gnome.org//browse/glib/tree/gobject/gtype.c#n1834

> Add reference counted strings
> ---
> 
>  • Documentation and introduction section are missing.
>  • Probably all need (skip) annotations.
>  • g_ref_string_new(): Why disallow the case of the empty string?
>  • Need an overflow check for (len + 1).
>  • Can we have a g_ref_string_new_len() which takes a string length to use,
> so we can easily extract substrings from other strings when creating
> ref-counted strings?

Yes, this is something that Richard asked as well.

>  • We won’t be able to use ref-counted strings with g_autofree any more, and
> similarly won’t be able to use them with g_autoptr or g_auto due to them not
> having a separate type name. We could either introduce a new g_autostr macro
> to allow them to be freed automatically; or could add a `typedef gchar
> GRefString` and `G_DEFINE_AUTOPTR_CLEANUP_FUNC (GRefString,
> g_ref_string_unref)`.

Yes, having a GRefString type would fit into the GStrv pattern that we already use.

(In reply to Emmanuele Bassi (:ebassi) from comment #90)
> (In reply to Philip Withnall from comment #89)
> > Patch review would be easier if the patches were on Bugzilla.
> 
> I know, but this bug is getting unwieldy already…
> 
> > Anyway, some comments:
>  
> Thanks!
> 
> I'm just going to reply to some of the issues; I agree with what I left out.
> 
> > Add reference counter API
> > ---
> > 
> >  • SECTION:refcount is missing introductory documentation and a ‘Since:’
> > line. The introductory documentation should make it very clear that
> > g_ref_counter_make_atomic() must only be called before an object’s refcount
> > has been exposed to other code (for example, in the object’s constructor),
> > otherwise there’s the possibility of a race between a non-atomic acquire()
> > call from some code outside the object, and the make_atomic() call.
> 
> The documentation part of the branch is completely up in the air, mostly
> because I didn't want to commit to it before getting a general review. Same
> goes for the tests.

I thought so, and that seems reasonable, but I didn’t want to leave it out of the review in case (somehow) it got forgotten.

> >  • Should the g_ref_counter_*() API be marked as (skip)?
> 
> Dunno; do we foresee some language other than C using integers as reference
> counters?

I don’t. It’s also pretty tied to C idioms (volatile, passing integers by reference). We can always un-mark the API as (skip) later if the need arises.

> >  • General comment about grefcount.c: nowhere does it actually document that
> > the internal representation uses positive ref-counts for non-atomic accesses
> > and negative ones for atomic accesses.
> 
> Should it be explicitly described? Do we want to commit to this strategy?

Sorry, I should have been clearer. I mean that it would be useful to have a comment in the code. That comment does not necessarily have to be in a public gtk-doc comment. As you say, it’s an implementation detail and best to publicly say that the integer values are opaque and that users of g_ref_counter_*() should not read or write them without using the g_ref_counter_*() API.

> >  • It would be useful to add opportunistic guards for functions which assert
> > that (ref_count != 0).
> 
> Good point, though I'd keep those assertions under a G_DISABLE_CHECKS
> conditional.

g_return_val_if_fail() is already a no-op if G_DISABLE_CHECKS is defined.

> > These all look good.
> > 
> > Add reference counted memory
> > ---
> 
> >  • The purpose of all the ALIGN_STRUCT(1) stuff isn’t clear to me. It seems
> > to be a 16-byte block of space which is never used, and whose size doesn’t
> > relate to the GRef structure size, so I’m not sure how it can be providing
> > alignment padding.
> 
> Again, I've lifted this code from what we do in GTypeInstance:
> https://git.gnome.org//browse/glib/tree/gobject/gtype.c#n1834

Ah, then the ALIGN_STRUCT(1) stuff would be because:
```
We also add extra private space at the start because
valgrind doesn't seem to like us claiming to have allocated an
address that it saw allocated by malloc().
```

It would be useful to mention this and/or add a comment pointing people to the comments in gtype.c. If you’re only going to use ALIGN_STRUCT(1) (rather than any other parameter value to it), you could clarify things by simply using a `#define INITIAL_VALGRIND_PADDING 16`.

I’ve been spending some time looking at this patch set, and have made some changes locally (which I’ll push in a branch somewhere soon, and write up).

However, while discussing it with Allison, it’s come up that neither of us are particularly sure about what the use cases are for these new APIs. What specific problems are they trying to solve?

For example, what benefits does g_ref_counted_*() bring over open-coding reference counting with ++/-- or g_atomic_int_[inc|dec_and_test]()? What is the use case for g_ref_counter_make_atomic()? Do all the use cases for atomic refcounting using g_ref_counter_*() also need a lock? If so, should the API include support for locking if the refcounter is atomic?

Are there other use cases for g_ref_*() other than for writing g_ref_string_*()? It can’t trivially be used to add reference counting to opaque structs from other libraries, for example, since we don’t have control over their new() and free() functions.

Is GRefString just there to reduce memory consumption? If so, why does it not also deduplicate the strings, like the AsRefString implementation in appstream-glib[1] does?

[1]: https://github.com/hughsie/appstream-glib/blob/master/libappstream-glib/as-ref-string.c

My branch is here: https://gitlab.com/pwithnall/glib/tree/622721-ref-string

Summary of changes:
 • Change g_ref_counter_*() to use the high bit to indicate whether to use atomics. This means we can use g_atomic_int_[inc|dec_and_test]() directly, rather than a custom CAS loop.
 • Introduce GClearNotify instead of GDestroyNotify, since we don’t want semantics which imply freeing the containing data structure.
 • Get rid of g_atomic_ref_*() since it seems pointless to have g_atomic_ref_[release|acquire]() have exactly the same implementation as g_ref_[release|acquire]().
 • Documentation everywhere except in GRefString.

I’m not planning on doing any more work on this until the questions in comment #92 are answered.

Thanks for looking into this.

(In reply to Philip Withnall from comment #92)
> I’ve been spending some time looking at this patch set, and have made some
> changes locally (which I’ll push in a branch somewhere soon, and write up).
> 
> However, while discussing it with Allison, it’s come up that neither of us
> are particularly sure about what the use cases are for these new APIs. What
> specific problems are they trying to solve?

So, let's back this up a bit, even though this bug is becoming increasingly unwieldy already.

The original intent (and what seems to be Richard's goal) was to have reference counted strings of unknown size, to be marshalled around in signals and properties without copying them all over the place. Usually those strings come from a storage that XML and have non-trivial lengths, so moving them around as pointers is not really safe (or usable, from bindings), and you cannot just intern them because you want to release the memory at then end.

On top of this, I wanted to get a bunch of code in various projects (Clutter being one of them, but GTK+ also applies) that has heap-allocated structures that should not be copied around, and turn them into boxed types with reference counting. Making them object just for that was pretty much overkill, but of course it's a possibility.

> For example, what benefits does g_ref_counted_*() bring over open-coding
> reference counting with ++/-- or g_atomic_int_[inc|dec_and_test]()? What is
> the use case for g_ref_counter_make_atomic()? Do all the use cases for
> atomic refcounting using g_ref_counter_*() also need a lock? If so, should
> the API include support for locking if the refcounter is atomic?

The whole discussion about atomic/non-atomic refcounting came after discussing this with Allison and other people at various hackfests. The original idea was to have two separate types; then it was collapsed into having a single type that could be transformed from non-atomic to atomic, atomically.

In general, we *really* want to stop using atomic refcounting inside GTK+; it's not buying us anything, and it's just a performance penalty for us.

To be fair, even the kernel is now getting a refcount_t type: https://lwn.net/Articles/715161/#refcount — to replace and do safety checks on top of their atomic type.

> Are there other use cases for g_ref_*() other than for writing
> g_ref_string_*()? It can’t trivially be used to add reference counting to
> opaque structs from other libraries, for example, since we don’t have
> control over their new() and free() functions.

I always envisioned this API to be consumed by those libraries, instead of writing their own reference counting code, which usually is buggy.

> Is GRefString just there to reduce memory consumption? If so, why does it
> not also deduplicate the strings, like the AsRefString implementation in
> appstream-glib[1] does?

De-duplication only makes sense if you know how big your strings are going to be, and if their size is small enough to compensate for the cost of comparing strings.

The main use case in rhythmbox is to reduce memory usage, when presenting the user with a database. A lot of strings will be duplicated for each media file (the album, and artist strings being the most likely ones), and increasing memory reuse through referenced strings lower the memory usage.

See:
https://git.gnome.org//browse/rhythmbox/tree/rhythmdb/rb-refstring.h

I'm sure Colin could go into more details ;)

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

Updated branch: https://git.gnome.org/browse/glib/log/?h=wip/ebassi/rc-new

This implements something similar to the kernel interface I linked in comment 94:

 - a simple reference count type, `grefcount` with an init(), inc(), dec(), and compare() API
 - an atomic reference count type, `gatomicrefcount` with an init(), inc(), dec(), and compare() API

You cannot mix the two — the atomic refcount is managed in Z⁺ (always positive), the non-atomic refcount is in Z¯ (always negative). Zero is used to determine the collection condition. Atomic reference counting is done using the g_atomic_int_* API.

In the branch I've ported a couple of GLib data structures to `gatomicrefcount` to show how it works.

Orthogonally to that, we can start working on reference counted strings using the "overallocate a string+len, and return in the middle of the struct" trick — and, eventually, an "RcBox/ArcBox" type that boxes any typically stack-allocated plain-old-data structure into a reference counted container, for things like signal handlers.

OK, some review comments:

New API
---

 • Copyright should be 2018 rather than 2017 (or both)
 • Documentation has some FIXME comments
 • g_ref_count_inc() is missing a check for wraparound when reading G_MININT (similarly for the other inc()/dec() functions)
 • I’m not totally sold on the compare() APIs — what’s the need for them? I see they’re used for an optimisation in GBytes and for an assertion in GHashTable. Refcounts are supposed to be opaque, though. Do you see a use case for them other than for these two things?
 • g_ref_count_compare() will require the caller to pass in a negative number to compare against, which somewhat breaks the abstraction.
 • Should gatomicrefcount be declared as `volatile`? That keyword has been causing some problems in bug #791706.


GHashTable changes
---

The compare() call looks dodgy:
-  g_assert (hash_table->ref_count > 0);
+  g_assert (g_atomic_ref_count_compare (&hash_table->ref_count, 0));

That’s gone from asserting that the ref_count is positive, to asserting that it’s zero.

Emmanuele and I discussed this a bit at the GTK+ hackfest, and decided that since we’re very near to the API freeze for 2.56, this should be punted to 2.58. However, we should aim to get it in early during that cycle.

-- GitLab Migration Automatic Message --

This bug has been migrated to GNOME's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/glib/issues/315.