It's not obvious why we wouldn't use g_quark_try_string(). Add a code
comment that this is intentional and a reference for how to find out
more.
Also, fix typo in another code comment.
The amount of used memory should stay in relation to the number of
entries we have. If we delete most (75%) of the entries, let's also
reallocate the buffer down to 50% of its size.
datalist_append() now starts with 2 elements. This works together with
the shrinking. If we only have one entry left, we will shrink the buffer
back to size 2. In general, d->alloc is always a power of two (unless it
overflows after G_MAXUINT32/2, which we assume will never happen).
The previous buffer growth strategy of never shrinking is not
necessarily bad. It has the advantage to not require any checks for
shrinking, and it works well in cases where the amount of data actually
does not shrink (as we'd often expect).
Also, it's questionable what a realloc() to a smaller size really
brings. Is that really gonna help and will the allocator do something
useful?
Anyway. This patch introduces shrinking. The check for whether to shrink
changes from `if (d->len == 0)` to `if (d->len <= d->alloc / 4u)`, which
is probably cheap even if most of the time we don't need to shrink. For
most cases, that's the only change that this patch brings. However, once
we find out that 75% of the buffer are empty, calling realloc() seems a
sensible thing to do.
The main point here is to reuse datalist_remove() and datalist_shrink().
Especially, datalist_shrink() will become more interesting next, when it
actually shrinks the buffer.
Also, I find the previous implementation with "data_end" confusing.
Instead, only use index "i_data" to iterate over the data.
Extract helper functions datalist_remove() and datalist_shrink(). This
is to reduce duplicate code, but also to have a default way how to do
this.
In particular, later datalist_shrink() might do more aggressive
shrinking. We need to have that code in one place.
g_datalist_unlock() is probably faster than g_datalist_unlock_and_set().
Move the "if (data)" check (that we anyway had) earlier, so we can
call g_datalist_unlock() and return early.
If too many keys are requested, they temporary buffer is allocated
on the heap. There is no problem in principle, to remove more than
16 keys.
Well, the problem is that GData tracks entries in a linear list, so
performance will degrade when it grows too much. That is a problem,
and users should be careful to not add unreasonably many keys. But it's
not the task of g_datalist_id_remove_multiple() to decide what is
reasonable.
This limitation was present from the beginning, in commit 0415bf9412
('Add g_datalist_id_remove_multiple'). It's no longer necessary since
commit eada6be364 ('gdataset: cleanup g_data_remove_internal()').
GDataSet is mainly used by GObject. Usually, when we access the private
data there, we already hold another lock around the GObject.
For example, before accessing quark_toggle_refs, we take a
OPTIONAL_BIT_LOCK_TOGGLE_REFS lock. That makes sense, because we anyway
need to protect access to the ToggleRefStack. By holding such an
external mutex around several GData operations, we achieve atomic
updates.
However, there is a (performance) use case to update the qdata
atomically, without such additional lock. The GData already holds a lock
while updating the data. Add a new g_datalist_id_update_atomic()
function, that can invoke a callback while holding that lock.
This will be used by GObject. The benefit is that we can access the
GData atomically, without requiring another mutex around it.
For example, a common pattern is to request some GData entry, and if
it's not yet allocated, to allocate it. This requires to take the GData
bitlock twice. With this API, the callback can allocate the data if no
entry exists yet.
In all cases after taking the lock with g_datalist_lock(), we also
get the pointer. And it hardly makes sense otherwise.
Replace g_datalist_lock() by g_datalist_lock_and_get() which does
both steps in one.
- only use gnewa0() for up to 400 bytes (arbitrarily chosen as
something that is probably small enough to cover most small tasks
while fitting easily on the stack). Otherwise fallback to g_new0().
- don't do intermediate G_DATALIST_SET_POINTER(). Set to NULL
afterwards with g_datalist_unlock_and_set().
- move the g_free(d) after releasing the lock on datalist.
- when setting datalist to NULL, do it at the end with
g_datalist_unlock_and_set() to avoid the additional atomic
operations.
- when freeing the old "d", do that after releasing the bit
lock.
The previous code was in practice correct.
(((guintptr) ptr) | ((gsize) mask))
works even if gsize is smaller than guintptr. And while the C standard
only guarantees that cast between uintptr_t and void* works, on
reasonable compilers it works to directly cast uintptr_t to GData*.
Still, no need for such uncertainty. Just be clear about the involved
types (use guintptr throughout) and cast first to (gpointer).
g_datalist_clear_i() had only one caller, altough the code comment made
it sound as if in the past there were more.
A function that has only one caller, and then needs a code comment to
explain the context in which it is called, makes the code more complicated
than necessary.
Especially since the function expects to be called with a global lock
held, and then unlocks and relocks. Spreading such things to another
function (which is only used once) makes code harder to follow.
Inline the function, so that we can see all the (trivial) code at one place.
This saves an additional atomic set.
Also, changing the pointer before the unlock can awake a blocked thread,
but at this point, the lock is still held (and the thread may need to
sleep again). This can be avoided.
Move them to a separate page as they don’t really have a ‘class’ struct
each to hang docs off.
Signed-off-by: Philip Withnall <pwithnall@gnome.org>
Helps: #3037
Round-tripping pointers via gsize is not guaranteed to work (the C standard
only requires this for (u)inptr_t) and in fact breaks on CHERI-enabled
systems such as Arm Morello where pointers are 128-bits but size_t is 64.
This means the current casts would strip the high bits of the pointer and
return a non-dereferenceable value. Fix this by casting the operand that
holds the pointer to guintptr instead of gsize.
Helps: https://gitlab.gnome.org/GNOME/glib/-/issues/2842
As per commit 5d738ddc some callbacks are not nullable anymore even
though they should be.
This breaks the introspection as some arguments won't be considered
nullable anymore.
You can notice this in https://gitlab.gnome.org/3v1n0/gjs/-/pipelines/558839
where some some tests are running against stable g-i version and others
(the failing ones) against the devel one.
Following Emmanuele's instructions for use of introspection annotations:
https://www.bassi.io/articles/2023/02/20/bindable-api-2023/
I have audited all uses of the (closure) annotation in glib and
determined that only a handful are correct. This commit changes almost
all of our use of (closure) annotations to conform to Emmanuele's rules.
In `g_data_remove_internal()`, call the `GDataElt:destroy` functions in the
order that they appear in `keys`, instead of the order that they are found in
`datalist`.
Fixes https://gitlab.gnome.org/GNOME/glib/-/issues/2672
This is more efficient than calling
g_datalist_id_remove() multiple times
in a row, since it only takes the locks
once.
Allow up to 16 keys to be removed in one go.
That is enough for the use we have in GObject,
and it avoids any danger of blowing the stack.
Add SPDX license (but not copyright) headers to all files which follow a
certain pattern in their existing non-machine-readable header comment.
This commit was entirely generated using the command:
```
git ls-files glib/*.[ch] | xargs perl -0777 -pi -e 's/\n \*\n \* This library is free software; you can redistribute it and\/or\n \* modify it under the terms of the GNU Lesser General Public/\n \*\n \* SPDX-License-Identifier: LGPL-2.1-or-later\n \*\n \* This library is free software; you can redistribute it and\/or\n \* modify it under the terms of the GNU Lesser General Public/igs'
```
Signed-off-by: Philip Withnall <pwithnall@endlessos.org>
Helps: #1415
glib/gdataset.c: In function ‘g_datalist_clear_i’:
glib/gdataset.c:233:21: error: comparison of integer expressions of different signedness: ‘gint’ {aka ‘int’} and ‘guint32’ {aka ‘unsigned int’} [-Werror=sign-compare]
for (i = 0; i < data->len; i++)
^
glib/gdataset.c: In function ‘g_datalist_clear’:
glib/gdataset.c:270:21: error: comparison of integer expressions of different signedness: ‘gint’ {aka ‘int’} and ‘guint32’ {aka ‘unsigned int’} [-Werror=sign-compare]
for (i = 0; i < data->len; i++)
^
glib/gdataset.c: In function ‘g_datalist_foreach’:
glib/gdataset.c:1147:21: error: comparison of integer expressions of different signedness: ‘int’ and ‘guint32’ {aka ‘unsigned int’} [-Werror=sign-compare]
for (j = 0; j < d->len; j++)
^
The functions with a GDestroyNotify to the data, or other ill-fitting
allocation semantics, are not currently introspectable.
The effect for the binding user would be that they're unable to
create or destroy a GData list, but they might still have an API
to poke at some data pointers from it.
In fact, none of the functions dealing with GData** or a dataset
location pointer are likely to get sensible bindings anyway;
the annotations added are mostly to avoid memory unsafety
and leaks.
https://bugzilla.gnome.org/show_bug.cgi?id=756470
All glib/*.{c,h} files have been processed, as well as gtester-report.
12 of those files are not licensed under LGPL:
gbsearcharray.h
gconstructor.h
glibintl.h
gmirroringtable.h
gscripttable.h
gtranslit-data.h
gunibreak.h
gunichartables.h
gunicomp.h
gunidecomp.h
valgrind.h
win_iconv.c
Some of them are generated files, some are licensed under a BSD-style
license and win_iconv.c is in the public domain.
Sub-directories inside glib/:
deprecated/: processed in a previous commit
glib-mirroring-tab/: already LGPLv2.1+
gnulib/: not modified, the code is copied from gnulib
libcharset/: a copy
pcre/: a copy
tests/: processed in a previous commit
https://bugzilla.gnome.org/show_bug.cgi?id=776504
If we have an input parameter (or return value) we need to use (nullable).
However, if it is an (inout) or (out) parameter, (optional) is sufficient.
It looks like (nullable) could be used for everything according to the
Annotation documentation, but (optional) is more specific.
Add various (nullable) and (optional) annotations which were missing
from a variety of functions. Also port a couple of existing (allow-none)
annotations in the same files to use (nullable) and (optional) as
appropriate instead.
Secondly, add various (not nullable) annotations as needed by the new
default in gobject-introspection of marking gpointers as (nullable). See
https://bugzilla.gnome.org/show_bug.cgi?id=729660.
This includes adding some stub documentation comments for the
assertion macro error functions, which weren’t previously documented.
The new comments are purely to allow for annotations, and hence are
marked as (skip) to prevent the symbols appearing in the GIR file.
https://bugzilla.gnome.org/show_bug.cgi?id=719966
This code is used in the property notification path, so it
better be fast. This commit removes a g_return_if_fail check and
treats the common case of just a single data element better.
Also, make it possible to get a 'new ref' on a datalist member
in a race-free way.
This is useful when using object data in thread-safe libraries.
https://bugzilla.gnome.org/show_bug.cgi?id=682849
