Note that OBJECT_HAS_TOGGLE_REF() is no longer used. This is good and
bad.
What we soon will do, is drop the OPTIONAL_BIT_LOCK_TOGGLE_REFS lock,
and only call g_datalist_id_update_atomic(). This way, we only need to
take the GData lock.
However, g_datalist_id_update_atomic() also needs to do a linear search
for quark_toggle_refs key. Previously, with OBJECT_HAS_TOGGLE_REF() we
didn't require this.
So the new approach is only faster, if searching the GQuark key is
reasonably fast. As GData tracks the objects in a linear list, adding
many keys can degrade and result in bad performance.
But the solution for that problem is to improve GData so that it's fast
enough for our purposes. Saying we cannot use it because it's too slow
to find the key, is wrong.
Previously, at two places (in g_object_real_dispose() and shortly before
finalize()), we would call
g_datalist_id_set_data (&object->qdata, quark_weak_notifies, NULL);
This clears @quark_weak_notifies at once and then invokes all
notifications.
This means, if you were inside a notification and called
g_object_weak_unref() on the object for *another* weak-reference, then
an exception failed:
GLib-GObject-FATAL-CRITICAL: g_object_weak_unref_cb: couldn't find weak ref 0x401320(0x16b9fe0)
Granted, maybe inside a GWeakNotify you shouldn't call anything on
where_the_object_was. However, unregistering things (like calling
g_object_weak_unref() should still reasonably work.
Instead, now remove each weak notification one by one and invoke it.
https://gitlab.gnome.org/GNOME/glib/-/issues/1002
Note that we have two calls
g_datalist_id_set_data (&object->qdata, quark_weak_notifies, NULL);
that don't take OPTIONAL_BIT_LOCK_WEAK_REFS lock. One is right before
finalize(). At that point, no other thread can hold a reference to the
object to race. One call is from g_object_real_dispose(). At this point,
theoretically the object could have been resurrected and a pointer
passed to another thread, so it can race against g_object_weak_ref()
and g_object_weak_unref().
Fix that, by performing all operations on the WeakRefStack while holding
the GData lock. By using g_datalist_id_update_atomic(), we no longer
need the OPTIONAL_BIT_LOCK_WEAK_REFS lock. On the other hand, we might
now do slightly more work while holding the GData lock.
Also, during g_object_weak_unref() free the WeakRefStack, if there
are no more references.
There are two calls to
g_datalist_id_set_data (&object->qdata, quark_closure_array, NULL);
that don't take a OPTIONAL_BIT_LOCK_CLOSURE_ARRAY lock.
One is right before finalize(), at which point it's most likely safe, as
nobody else should have a reference to the object to do anything
concurrently.
The other one is in g_object_real_dispose(). Usually, at this point too
no other thread has have a reference to the object. However, it could
obtain one by dispose() passing on a reference. In that case, the
removal of the quark_closure_array data can race against
g_object_watch_closure() and object_remove_closure().
Fix that, by having all operations on the CArray performed under a lock.
In this case, using the GData lock via g_datalist_id_update_atomic().
This also saves the additional OPTIONAL_BIT_LOCK_CLOSURE_ARRAY lock.
At the expense of doing slightly more work while holding the GData lock.
Also free the empty closure data during object_remove_closure(). This
frees some unused memory.
This allows the caller to take the lock on the GData first, and perform
some operations.
This is useful under the assumption, that the caller can find cases
where calling g_datalist_id_update_atomic() is unnecessary, but it's
still necessary to perform that check while holding a lock.
That will yield better performance, if we can avoid calling
g_datalist_id_update_atomic(). That matters for checking the
toggle-notify in g_object_ref()/g_object_unref().
Note that with "already_locked", g_datalist_id_update_atomic() will
still unlock the GData at the end. That is because the API of
g_datalist_id_update_atomic() requires that it might re-allocate the
buffer, and it can do a more efficient unlock in that case, instead of
leaving it to the caller. The usage and purpose of this parameter is
very special, so this asymmetry is taken because the only callers will
be fine with this behavior, and it results in potentially more efficient
unlocking.
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.
`@filename@` expands to the (absolute or relative) path from the
build directory to the source directory, which can be rather verbose.
In practice Meson usually (always?) generates a relative path, but
even so, the resulting installed header is not necessarily reproducible
if using different build directories outside the source directory.
We don't really need a full path here anyway: the basename is enough
of a hint to point a reader towards the file where the underlying
enum was defined.
Signed-off-by: Simon McVittie <smcv@collabora.com>
The docs for the constructed vfunc make it clear that when called
constructed properties are already set. However, the first place where a
user would look for is the flag's documentation.
It’s a false positive, but points to a slightly unnecessary use of a
global variable to store something which could be computed per-test.
scan-build thought that arrays of size `n_handlers` could have
uninitialised values accessed in them if `n_handlers` were to change
value part-way through a test (which it didn’t).
Signed-off-by: Philip Withnall <pwithnall@gnome.org>
It’s deprecated, but I was modifying it anyway and it didn’t have any
coverage, so let’s add a simple test (as suggested by Michael
Catanzaro).
Signed-off-by: Philip Withnall <pwithnall@gnome.org>
The latter only accepts a `gint` as the number of elements in the array,
which means that its use in `GArray` (and related array implementations)
truncates at least half the potential array size.
So, introduce a replacement for it which uses `size_t` for the number of
elements. This is inline with what `qsort()` (or `qsort_r()`) actually
does. Unfortunately we can’t directly use `qsort_r()` because it’s not
guaranteed to be a stable sort.
This fixes some `-Wsign-conversion` warnings (when building GLib with
that enabled).
Signed-off-by: Philip Withnall <pwithnall@gnome.org>
Helps: #3405
If we enable `-Wfloat-conversion`, these warn about a possible loss of
precision due to an implicit conversion from `double` to some other
numeric type.
The warning is correct: there is a possible loss of precision here. In
these instances, we don’t care, as the floating point arithmetic is
being done to do some imprecise scaling or imprecise timing. A loss of
precision is not a problem.
So, add an explicit cast to squash the warning.
Signed-off-by: Philip Withnall <pwithnall@gnome.org>
Helps: #3405
The tests were using a lot of signed `int`s when actually the values
being handled were always non-negative. Use `unsigned int` consistently
throughout.
Take the opportunity to move declarations of loop iterator variables
into the loops.
This introduces no functional changes.
Signed-off-by: Philip Withnall <pwithnall@gnome.org>
Helps: #3405
There were various places where (signed or unsigned) integer assertions
were being used to compare `double` or `float` values, resulting in an
implicit integer conversion.
This causes a warning when building with `-Wfloat-conversion`.
Improve the specificity of the tests by using the most-specific numeric
assertions through all `param` tests.
For the conversion tests, this means using the assertion function
associated with the target type, not the source type.
Signed-off-by: Philip Withnall <pwithnall@gnome.org>
Helps: #3405
Compiling with `-Wfloat-conversion` warns about a few implicit
conversions from `double`/`float` to other numeric types in the `GValue`
transform functions.
These warnings are correct: value transformations can result in loss of
precision. That loss of precision is understood and expected, so add
some explicit casts to squash the warnings.
Signed-off-by: Philip Withnall <pwithnall@gnome.org>
Helps: #3405
Rather than defining it as a double constant. This introduces no
functional changes, but does squash some `-Wfloat-conversion` warnings.
Signed-off-by: Philip Withnall <pwithnall@gnome.org>
Helps: #3405
As spotted by `-Wfloat-conversion`. Doubles which could not be
accurately represented as floats may have erroneously failed bounds
validation.
Signed-off-by: Philip Withnall <pwithnall@gnome.org>
Helps: #3405
The default values for construct properties always have to be set, even
if those properties are deprecated. The code to do that is in GLib, and
not under the control of the user (unless they completely override the
`constructor` vfunc, which is not recommended). So don’t emit a warning
for that if `G_ENABLE_DIAGNOSTICS` is enabled.
In particular, this fixes deprecation warnings being emitted for
properties of a parent class when chaining up with a custom constructor,
even when none of the child class code mentions the deprecated property.
Signed-off-by: Philip Withnall <pwithnall@gnome.org>
Fixes: #3254
This just makes it a bit clearer that they’re atomic/for thread safety,
and not just NIHed bit operations with shouty names.
This introduces no functional changes.
Signed-off-by: Philip Withnall <pwithnall@gnome.org>
This avoids the need to ref/unref the closure while invalidating it in
the `closure->ref_count == 1` path in `g_closure_unref()`.
scan-build gets very confused about the ref count here, and ends up
assuming it’s possible for the `g_closure_unref()` call in
`g_closure_invalidate()` to finalise the closure when the latter is
called from `g_closure_unref()`. There was an existing assertion in
`g_closure_invalidate()` which hinted that this wasn’t possible, but
scan-build doesn’t seem to be able to propagate assumptions about
refcounts between function contexts.
So, introduce an internal variant of `g_closure_invalidate()` which can
skip modifying the closure’s refcount. It’s safe to invalidate the
closure without adding a ref when doing so from `g_closure_unref()` with
`closure->ref_count == 1` because at that point `g_closure_unref()`
holds the only remaining ref to the closure. So none of the invalidation
callbacks are allowed to unref it further.
Signed-off-by: Philip Withnall <pwithnall@gnome.org>
Helps: #1767
scan-build is worried that `node->data->common.value_table->value_init`
will be a `NULL` pointer dereference in the assignment to
`node->mutatable_check_cache`.
There’s already an assertion immediately below to check against this, so
let’s move it up a line to help the static analyser out.
Signed-off-by: Philip Withnall <pwithnall@gnome.org>
Helps: #1767
Avoid scan-build thinking that `new_wrdata` could be `NULL` on this
control path. It can’t be `NULL` if `new_object` is set.
Signed-off-by: Philip Withnall <pwithnall@gnome.org>
Helps: #1767
Basically various trivial instances of the following MSVC compiler
warning:
```
../gio/gio-tool-set.c(50): warning C4267: '=': conversion from 'size_t' to 'int', possible loss of data
```
Signed-off-by: Philip Withnall <pwithnall@gnome.org>
I’m not sure exactly how this code is supposed to work, so this might
not be the right fix. But there’s definitely a problem here, and it was
spotted by scan-build.
If `param_value_array_validate()` is entered with
`value->data[0].v_pointer == NULL && aspec->fixed_n_elements`, that `NULL`
will be stored in `value_array` too. `value->data[0].v_pointer` will
then be set to a new non-`NULL` array.
A few lines down, `value_array_ensure_size()` is called on
`value_array` – which is still `NULL` – and this results in a `NULL`
pointer dereference.
It looks like `value->data[0].v_pointer` and `value_array` are used
interchangeably throughout the whole of the function, so assign the new
value of `value->data[0].v_pointer` to `value_array` too.
My guess is that `value_array` is just a convenience alias for
`value->data[0].v_pointer`, because the latter is a real mouthful to
type or read.
Signed-off-by: Philip Withnall <pwithnall@gnome.org>
Helps: #1767
