It’s reasonable for the `main()` function in a test suite to pass
ownership of some test data to `g_test_add_data_func_full()`, along with
a `GDestroyNotify`, and rely on GTest to free the data after all tests
have been run.
Unfortunately that only worked if the test was run, and not skipped
before its test function was called. This could happen if, for example,
it had `/subprocess` in its path.
Fix that by always freeing the test data. This required reworking how
tests are skipped, slightly, to bring all the logic for that within
`test_case_run()`, so that it could always handle the memory management.
Signed-off-by: Philip Withnall <pwithnall@gnome.org>
Helps: #3248
It’s more helpful to always register the test, even if it’s normally
skipped, since then the skip is recorded in the test logs so people can
see what’s ‘missing’ from them.
Signed-off-by: Philip Withnall <pwithnall@gnome.org>
The function arguments index_ and length could lead to a sum which is
larger than G_MAXUINT, possibly leading to out of boundary accesses
in array_remove_range functions.
Signed-off-by: Tobias Stoeckmann <tobias@stoeckmann.org>
Fixes: #3240
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.
So that it matches `gi_arg_info_get_type_info()`. We can’t use
`gi_arg_info_get_type()` because that collides with the `GType` getter
for the type.
Spotted by Philip Chimento.
Signed-off-by: Philip Withnall <pwithnall@gnome.org>
Fixes: #3243
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.
GI_IS_REGISTERED_TYPE_INFO() wasn't working because it was actually
defined to be the same as GI_IS_OBJECT_INFO().
Add some desultory type-checking assertions to the repository tests.
gi_repository_enumerate_versions() was missing a type check of the
instance parameter. This helps catch mistakes when porting from
girepository 1.x where the parameter was allowed to be null.
Memory was leaking when allocating it inside libelf and losing the pointer to it (it was an automatic variable) when returning NULL from the get_elf function in some cases
Closes#3242
Signed-off-by: Maxim Moskalets <Maxim.Moskalets@kaspersky.com>
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()').
GSList doesn't seem the best choice here. It's benefits are that it's
relatively convenient to use (albeit not very efficient) and that an
empty list requires only the pointer to the list's head.
But for non-empty list, we need to allocate GSList elements. We can do
better, by writing more code.
I think it's worth optimizing GObject, at the expense of a bit(?) more
complicated code. The complicated code is still entirely self-contained,
so unless you review WeakRefData usage, it doesn't need to bother you.
Note that this can be easily measure to be a bit faster. But I think the
more important part is to safe some allocations. Often objects are
long-lived, and the GWeakRef will be tracked for a long time. It is
interesting, to optimize the memory usage of that.
- if the list only contains one weak reference, it's interned/embedded in
WeakRefData.list.one. Otherwise, an array is allocated and tracked
at WeakRefData.list.many.
- when the buffer grows, we double the size. When the buffer shrinks,
we reallocate to 50% when 75% are empty. When the buffer shrinks to
length 1, we free it (so that "list.one" is always used with a length
of 1).
That means, at worst case we waste 75% of the allocated buffer,
which is a choice in the hope that future weak references will be
registered, and that this is a suitable strategy.
- on architectures like x86_68, does this not increase the size of
WeakRefData.
Also, the number of weak-refs is now limited to 65535, and now an
assertion fails when you try to register more than that. But note that
the internal tracking just uses a linear search, so you really don't
want to register thousands of weak references on an object. If you do
that, the current implementation is not suitable anyway and you must
rethink your approach. Nor does it make sense to optimize the
implementation for such a use case. Instead, the implementation is
optimized for a few (one!) weak reference per object.
We can safely combine this, and use bit 30 of the ref-count for locking.
This leaves still 2^30-1 for the ref-count, which is more than enough,
because these references are only taken for a short time in
g_weak_ref_get() and g_weak_ref_set(). Note that one thread can at most
take one reference at a time, so the ref-count will always a smaller
number.
Also note, that obviously we will only take a bit lock while also
holding a reference. That means, when weak_ref_data_unref() decreases
the ref-count to zero, the bit will be unlocked as well.
The reason to do this is to free up some space in WeakRefData. Note that
(on x86_64) this doesn't actually make the struct smaller. It's
probably not reasonably possible to make WeakRefData smaller than it
already is (on x86_64). However, by combining the fields we have some
space for reuse without increasing the struct size. That space will be
used next.
The implementation of GWeakRef tracks weak references in a way, that
requires linear search. That is probably best, for an expected low
number of entries (e.g. compared to the overhead of having a hash
table). However, it means, if you create thousands of weak references,
performance start to degrade.
Add a test that creates 64k weak references. Just to see how it goes.
This reverts commit fbdc9a2d03.
It was not submitted through a merge request and broke CI. Reverting it
immediately to unbreak CI and hence the rest of the development
pipeline. The changes can be re-submitted as a merge request so they’re
properly tested in CI before being merged.
See https://gitlab.gnome.org/GNOME/glib/-/merge_requests/3857#note_1994336
This might help increase visibility of Philip's useful GMainContext
tutorial. Although the GMainContext documentation is fairly good, it's
also pretty intimidating. The tutorial is very useful and provides
guidance that we can't fit directly into the documentation, so reference
it.
Replace the global RWLock with per-object locking. Note that there are
three places where we needed to take the globlal lock. g_weak_ref_get(),
g_weak_ref_set() and in _object_unref_clear_weak_locations(), during
g_object_unref(). The calls during g_object_unref() seem the most
relevant here, where we would want to avoid a global lock. Luckily, that
global lock only had to be taken if the object ever had a GWeakRef
registered, so most objects wouldn't care. The global lock only affects
objects, that are ever set via g_weak_ref_set(). Still, try to avoid that
global lock.
Related to GWeakRef, there are various moments when we don't hold a
strong reference to the object. So the per-object lock cannot be on the
object itself, because when we want to unlock we no longer have access
to the object. And we cannot take a strong reference on the GObject
either, because that triggers toggle notifications. And worse, when one
thread holds the last strong reference of an object and decides to
destroy it, then a `g_weak_ref_set(weak_ref, NULL)` on another thread
could acquire a temporary reference, and steal the destruction of the
object from the other thread.
Instead, we already had a "quark_weak_locations" GData and an allocated
structure for tracking the GSList with GWeakRef. Extend that to be
ref-counted and have a separate lifetime from the object. This
WeakRefData now contains the per-object mutex for locking. We can
request the WeakRefData from an object, take a reference to keep it
alive, and use it to hold the lock without having the object alive.
We also need a bitlock on GWeakRef itself. So to set or get a
GWeakRef we must take the per-object lock on the WeakRefData and the
lock on the GWeakRef (in this order). During g_weak_ref_set() there may
be of course two objects (and two WeakRefData) involved, the previous
and the new object.
Note that now once an object gets a WeakRefData allocated, it can no
longer be freed. It must stick until the object gets destroyed. This
allocation happens, once an object is set via g_weak_ref_set(). In
other words, objects involved with GWeakRef will have extra data
allocated.
It may be possible to also release the WeakRefData once it's no longer
needed. However, that would be quite complicated, and require additional
atomic operations, so it's not clear to be worth it. So it's not done.
Instead, the WeakRefData sticks on the object once it's set.
_object_unref_clear_weak_locations() is called twice during
g_object_unref(). In both cases, it is when we expect that the reference
count is 1 and we are either about to call dispose() or finalize().
At this point, we must check for GWeakRef to avoid a race that the ref
count gets increased just at that point.
However, we can do something better than to always take the global lock.
On the object, whenever an object is set to a GWeakRef, set a flag
OPTIONAL_FLAG_EVER_HAD_WEAK_REF. Most objects are not involved with weak
references and won't have this flag set.
If we reach _object_unref_clear_weak_locations() we just (atomically)
checked that the ref count is one. If the object at this point never had
a GWeakRef registered, we know that nobody else could have raced against
obtaining another reference. In this case, we can skip taking the lock
and checking for weak locations.
As most object don't ever have a GWeakRef registered, this significantly
avoids unnecessary work during _object_unref_clear_weak_locations().
This even fixes a hard to hit race in the do_unref=FALSE case.
Previously, if do_unref=FALSE there were code paths where we avoided
taking the global lock. We do so, when quark_weak_locations is unset.
However, that is not race free. If we enter
_object_unref_clear_weak_locations() with a ref-count of 1 and one
GWeakRef registered, another thread can take a strong reference and
unset the GWeakRef. Then quark_weak_locations will be unset, and
_object_unref_clear_weak_locations() misses the fact that the ref count
is now bumped to two. That is now fixed, because once
OPTIONAL_FLAG_EVER_HAD_WEAK_REF is set, it will stick.
Previously, there was an optimization to first take a read lock to check
whether there are weak locations to clear. It's not clear that this is
worth it, because we now already have a hint that there might be a weak
location. Unfortunately, GRWLock does not support an upgradable lock, so
we cannot take an (upgradable) read lock, and when necessary upgrade
that to a write lock.
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.
There are a handful of APIs in libgirepository which are used on
performance-sensitive code paths in language bindings (such as looking
at arguments when doing function calls). Historically libgirepository
has provided a stack-allocated variant for them, which avoids returning
a newly allocated `GIBaseInfo`. Since moving to glib.git and porting to
`GTypeInstance`, that stack allocated version has been broken.
This commit fixes it, by exposing obfuscated stack allocatable versions
of `GITypeInfo` and `GIArgInfo`, which are the two `GIBaseInfo`
subtypes which can be returned by the stack allocation functions.
The commit includes unit tests for them.
Signed-off-by: Philip Withnall <pwithnall@gnome.org>
Fixes: #3217
The previous commit enabled the `/run/mount/utab` monitoring. The problem
is that the `mount-changed` signal can be emitted twice for one mount. One
for the `/proc/mounts` file change and another one for the `/run/media/utab`
file change. This is still not ideal because e.g. the `GMount` objects for
mounts with the `x-gvfs-hide` option are added and immediately removed.
Let's enable the `mnt_monitor_veil_kernel` option to avoid this.
Related: https://github.com/util-linux/util-linux/pull/2725
The `GUnixMountMonitor` object implements monitoring on its own currently.
Only the `/proc/mounts` file changes are monitored. It is not aware of the
`/run/mount/utab` file changes. This file contains the userspace mount
options (e.g. `x-gvfs-notrash`, `x-gvfs-hide`) among others. There is a
problem when `/sbin/mount.<type>` (e.g. `mount.nfs`) helper programs are
used. In that case, the `/run/mount/utab` file is updated later than the
`/proc/mounts` file and thus the `GUnixMountMonitor` clients (e.g.
`gvfs-udisks2-volume-monitor`, `gvfsd-trash`) don't see the userspace
options until the next `mount-changed` signal. Let's use the `libmnt_monitor`
API for monitoring instead and emit the `mount-changed` signal also when the
`/run/mount/utab` file is changed.
Related: https://issues.redhat.com/browse/RHEL-14607
Related: https://github.com/util-linux/util-linux/pull/2607
It's not clear what this code comment tries to tell us. Yes, when we
make changes, we must take care that the changes are correct and update
the relevant places.
It seems long obsolete. Drop it.
This partly reverts commit d7dd9aefd8 ('placed a comment about not
changing CArray until we have').
