During object initialization, we may want to freeze the notifications,
but only do so once (and once unfreeze at the end).
Rework how that was done. We can avoid an additional GData lookup.
By now, GObjectNotifyQueue gets reallocated. So quite possibly if we
keep the queue, it is a dangling pointer.
That is error prone, but it's also unnecessary. All we need to know is
whether we bumped the freeze count and need to unfreeze. The queue
itself was not useful, because we anyway must take a lock (via
g_datalist_id_update_atomic()) to do anything with it.
Instead, use a nqueue_is_frozen boolean variable.
GSList is almost in all use cases a bad choice. It's bad for locality
and requires a heap allocation per entry.
Instead, use an array, and grow the buffer exponentially via realloc().
Now, that we use g_datalist_id_update_atomic(), it is also easy to
update the pointer. Hence, the GObjectNotifyQueue struct does not point
to an array of pspecs. Instead the entire GObjectNotifyQueue itself gets
reallocated, thus saving one heap allocation for the separate head
structure.
Now all accesses to quark_notify_queue are guarded by the GData lock.
Several non-trivial operations are implemented via
g_datalist_id_update_atomic().
The OPTIONAL_BIT_LOCK_NOTIFY lock is thus unnecessary and can be dropped.
Note that with the move to g_datalist_id_update_atomic(), we now
potentially do more work while holding the GData lock (e.g. some code
paths allocation additional memory). But note that
g_datalist_id_set_data() already has code paths where it must allocate
memory to track the GDataElt. Also, most objects are not used in
parallel, so holding the per-object (per-GData) lock longer does not
affect them. Also, many operations also require a object_bit_lock(), so
it's unlikely that you really could achieve higher parallelism by taking
more locks (and minimizing the time to hold the GData lock).
A common pattern is to look whether a GData entry exists, and if it
doesn't, add it.
For that, we currently always must take a OPTIONAL_BIT_LOCK_NOTIFY lock.
This can be avoided, because GData already uses an internal mutex. By
using g_datalist_id_update_atomic(), we can perform all relevant
operations while holding that mutex.
Move functionality from g_object_notify_queue_freeze() inside
g_datalist_id_update_atomic().
The goal will be to drop the OPTIONAL_BIT_LOCK_NOTIFY lock in a later
commit.
Cache the function pointer for g_datalist_id_update_atomic() in a static
variable to avoid looking it up repeatedly.
g_datalist_id_update_atomic() is anyway internal API. And just like
GData is not a very useful data structure in general, it's mainly used
by GObject. And since GObject is the at the code, optimizing this makes
sense.
We can easily see that we never call g_datalist_id_update_atomic()
without having a GObject at hand (because we call it on
`&object->qdata`. Thus initializing the pointer in
g_object_do_class_init() (under lock) works perfectly fine. And if this
invariant ever changes, you will get a crash immediately and notice the
problem.
None of the users actually care about this parameter. And it's unlikely
that they ever will. Also, the passed "key_id" is the argument from
g_datalist_id_update_atomic(). If the caller really cared to know the
"key_id" in the callback, they could pass it as additional user data.
Port the rest of the document to gi-docgen syntax, so we don’t have to
track partially-ported files.
Signed-off-by: Philip Withnall <pwithnall@gnome.org>
Helps: #3250
GData is a thread safe container. For that reason, it probably is anyway
not suitable to add 4 billion entries. That is even with the new index,
where we now scale with O(1).
Still, the code should not just break down at half of what we
theoretically could add.
Handle overflow and huge sizes correctly.
g_datalist_get_data() tries to avoid g_quark_try_string() and instead
do the lock-less g_quark_to_string(). For small number of entries that
may perform better. At least, this was an optimization from the past.
However, if we have a GHashTable index, and presumably a larger number
of entries, we should use the index instead. Unfortunately, that requires
us again to use g_quark_try_string() and take a global lock.
GData tracks the entries in a linear array, and performs a linear
search. That is very good, as long as the number of entries is small.
However, when it grows, performance gets worse. It's not clear what the
exact threshold is, or what the recommended maximum is.
Also, GData is used to attach arbitrary user data to GObject. Which is a
great feature, that a user might want to use. In that case, they may be
less concerned about performance to get the benefits of the feature, and
thus add more data than is best (though, it's unclear how much is too
much).
Also, GObject adds entries in its qdata. Hence, also for basic GObject
operations, it's important that this performs well. Note that access to
GData happens while holding a lock, we want to minimize the time while
holding that lock.
This patch ensures that access to GData is O(1) (and reasonably fast).
It thus allows to use GData in ways that wasn't advised previously.
There are alternatives, like using a binary search tree or always use a GHashTable.
Instead, we keep the linear buffer. Only when the buffer grows to
ALLOC_THRESHOLD_INDEX entries, we will start generating and maintaining
a GHashTable index. So the common case where objects have few entries
does not change. The memory overhead is only there, when the list grows.
ALLOC_THRESHOLD_INDEX is set to 64 entries. We will allocate such a
large buffer when adding the 33rd entry. During shrink, we will drop the
buffer again when shrinking down to 16 entries.
The reason for not always using the GHashTable is to save the memory in
common cases where there are few entries.
We use g_hash_table_add() to exploit the GHashTable optimization. We
also let it point to the GDataElt from the linear array. The benefit is
that we don't require individual allocations. The downside is that
during reallocation we need to regenerate the entire index.
In my tests this actually performs well. For example, following are
timings for calling g_dataset_id_get_data() in a loop. This is the
lookup time for an entry that doesn't exist. Obviously, the linear
search would scale always best when we would only lookup the first entry
in the list.
num-entries time-before time-after
1 0.144 0.145
2 0.146 0.146
5 0.149 0.150
10 0.155 0.157
20 0.172 0.170
32 0.248 0.254
33 0.249 0.184 <== index in use
40 0.284 0.183
50 0.317 0.189
75 0.370 0.184
100 0.442 0.183
300 1.044 0.186
1000 3.170 0.184
10000 31.597 0.189
This will be more useful next.
Also, try to detect whether realloc() actually moved the pointer. If it
doesn't, we can optimize a bit (not use g_datalist_unlock_and_set()).
Next, we will do more useful optimization in this case.
Note that we cannot just compare dangling pointers. That's would be
undefined behavior. In practice, we probably often compare dangling
pointers, and this tends to work just fine. Still avoid this and compare
only the guintptr values.
Avoid duplicated code.
Also, the "if (data->data[i].data && " check is unnecessary. The data
pointer is never NULL, because g_datalist_id_set_data() will treat that
as indication to remove the entry.
This is not strictly necessary, but helps scan-build understand that the
`g_thread_join()` calls are not going to use uninitialised `GThread`
pointers. It can’t reason that the number of loop iterations on the
`g_thread_create()` and `g_thread_join()` loops are identical.
Fixes:
```
../../../glib/tests/mutex.c:271:5: warning: 1st function call argument is an uninitialized value [core.CallAndMessage]
271 | g_thread_join (threads[i]);
| ^~~~~~~~~~~~~~~~~~~~~~~~~~
```
Signed-off-by: Philip Withnall <pwithnall@gnome.org>
This assertion basically mirrors the existing `buffer_size >= 2`
assertion (one implies the other), but scan-build doesn’t seem to be
able to work that out — so give it some help.
Fixes:
```
../../../glib/gfileutils.c: In function ‘g_get_current_dir’:
../../../glib/gfileutils.c:2995:17: warning: potential null pointer dereference [-Wnull-dereference]
2995 | buffer[1] = 0;
| ~~~~~~~~~~^~~
../../../glib/gfileutils.c:2994:17: warning: potential null pointer dereference [-Wnull-dereference]
2994 | buffer[0] = G_DIR_SEPARATOR;
```
Signed-off-by: Philip Withnall <pwithnall@gnome.org>
These symbols are documented in the {glib,gio}-{unix,win32}
documentation builds, and shouldn’t be duplicated in the main
documentation. It’s a historical (and unavoidable) accident that those
symbols are in the main GIR files.
If we don’t hide the symbols from the main docs build, we now run into
the problem of the `GioUnix` namespace not being known to gi-docgen
while building the `Gio` docs. This is because of the previous two
commits, which ported more of the `gunixmounts` documentation to
gi-docgen syntax. I don’t know of another way to fix this, other than to
hide the `GioUnix` symbols from the `Gio` docs build.
Signed-off-by: Philip Withnall <pwithnall@gnome.org>
Helps: #3250
Let’s not go with SHOUTY UNIX or quiet unix, let’s just call it what
Wikipedia calls it (https://en.wikipedia.org/wiki/Unix).
Signed-off-by: Philip Withnall <pwithnall@gnome.org>
They need a `GUnixMountMonitor` to give valid timestamps, and if you
have one of those then you might as well listen to its signals anyway.
Signed-off-by: Philip Withnall <pwithnall@gnome.org>
Fixes: #3569
If only someone would go ahead and invent a whole section of the list of
HTTP status codes which could be used to inform a client of where a
document has been moved to.
For the sake of argument, let’s say it could be status codes 300–399,
since they appear to be completely unused at the moment.
😩
Signed-off-by: Philip Withnall <pwithnall@gnome.org>
This used to happen consistently before !4290, but that MR changed it so
that `data` could be non-`NULL` if `size == 0` if the new inline code
path is taken.
While users of `GBytes` shouldn’t be dereferencing the data if the
bytes’ length is zero, it’s definitely safer to make sure the data is
`NULL` in that case.
This shouldn’t break the expectations of any third party code because
it’s restoring the behaviour from before !4290.
Signed-off-by: Philip Withnall <pwithnall@gnome.org>
Fixes: #3562