Fixes test timeouts like this one:
https://gitlab.gnome.org/GNOME/glib/-/jobs/4827270
The race will continue to be reproduced when running the tests not under
valgrind.
Signed-off-by: Philip Withnall <pwithnall@gnome.org>
Some tweakings of the time spend during warm up. That mostly matters if
you set very short "--seconds", which can make sense for quickly
checking something. Then the warmup should not take more thatn a certain
percentage of the requested runs.
When we have a constant factor, we still want not to run for more than
10% of the overall test time ... except, we still want to run at least
ESTIMATE_ROUND_TIME_N_RUNS (because we skip the estimation step below).
Also, adjust WARM_UP_ALWAYS_SEC to be only 20% of the test time, for
short test runs.
Also, don't print the messages about "Estimating round time" with a
fixed "--factor".
One test round aims to run for 8msec (TARGET_ROUND_TIME).
As the "--seconds" parameter previously took whole integer numbers, that
meant that we would run at least 125 rounds.
For a quick run, we should also support even faster runs, e.g. to select
only 0.5 seconds.
Historically, there was a verbose mode and a non-verbose mode.
In non-verbose mode (the default), we would still print two lines:
Running test property-set
Property set per second: 39329344
Later, this was changed to include the test name in the second line, so
we would print:
Running test property-set
property-set: Property set per second: 39329344
But this first line is really just noise, making parsing and reading the
results harder. Hence a "--quiet" mode was added, that only printed one
line per test while keeping the previous default behavior. And all was
good.
Except, unless you want verbose mode, this "Running test" line is still
not very useful and mainly clutters the output.
Supporess it now also in normal mode. It is now only printed in verbose
mode.
This also makes the "--quiet" option do nothing. The option is still
there, maybe we find a future use and we should not break the command
line API by dropping an argument.
g_object_set() optimizes the case where there are no signals connected.
Add a test that sets the property with signals. Obviously, this one is
much slower, since we will freeze and thaw the notifications.
For the test, we actually care to find the fastest test run (and take
"min_elapsed"). That is useful, because that is the run where we
possibly have the least interference from external factors, it was the
run where the CPU solved the problem as fast as it could.
As such, we should not reject the first 5% as additional warm up. If the
first 5% are slower (and part of "warmup"), then they are anyway not
considered. If there is a the fastest run in the first 5 percent, then
we want to take that.
Also note, that the calculation of "avg_elapsed" was wrong, since it
divided by the full "num_rounds" while only summing 95% of the runs.
This is fixed too by now considering all runs.
Fixes: 282d536fd229 ('tests/performance: ensure to always warm up for 2 seconds')
- fix and improve usage output for "performance-run.sh" script.
- add a sleep after compilation. It seems to me, that the first run
usually performs better, which might be because the temperature of the
CPU raises and the CPU gets throttled. Unclear whether that is really
the case, but add a sleep so that all runs start under similar
conditions where the CPU was idle for a moment.
When running the test (without parameters), it estimates a factor for
the run size for each test. That is useful for running a reasonable size
of the test, on different machines.
However, when comparing two runs, it seems important that both runs
share a common factor. Otherwise, the factor is determined differently,
and the test is less comparable. For that there is the "--factor" option
or the GLIB_PERFORMANCE_FACTOR environment variable.
However, the factor option can only set the factors for all tests at the
same time. Optimally, one factor is roughly suitable for all tests, but
it is not, as currently the detected factors on my machine are widely
different
$ ./build/gobject/tests/performance/performance -v > p
$ cat p | sed -n -e 's/^Running test //p' -e 's/.*correction factor //p' | sed 'N;s/\n/ /'
simple-construction 34.78
simple-construction1 145.45
complex-construction 11.08
complex-construction1 20.46
complex-construction2 23.74
finalization 4.74
type-check 37.74
emit-unhandled 5.63
emit-unhandled-empty 49.69
emit-unhandled-generic 7.17
emit-unhandled-generic-empty 50.63
emit-unhandled-args 5.20
emit-handled 3.86
emit-handled-empty 4.01
emit-handled-generic 3.96
emit-handled-generic-empty 7.04
emit-handled-args 3.78
notify-unhandled 52.63
notify-by-pspec-unhandled 156.86
notify-handled 2.55
notify-by-pspec-handled 2.66
property-set 34.63
property-get 32.92
refcount 0.83
refcount-1 2.30
refcount-toggle 1.33
Adjust the base factors with these measurements.
PERFORMANCE_FILE="./gobject/tests/performance/performance.c"
IFS=$'\n'
for LINE in $(cat p | sed -n -e 's/^Running test //p' -e 's/.*correction factor //p' | sed 'N;s/\n/ /') ; do
(
IFS=' '
set -- $LINE
TESTNAME="$1"
FACTOR="$2"
LINENUMBER="$(grep -n "^ \"$TESTNAME\",$" "$PERFORMANCE_FILE" | cut -d: -f1)"
LINENUMBER=$((LINENUMBER + 2))
OLD_FACTOR="$(sed -n "$LINENUMBER s/^ \([0-9]\+\),$/\1/p" "$PERFORMANCE_FILE")"
NEW_FACTOR="$(awk -v factor="$FACTOR" -v old_factor="$OLD_FACTOR" 'BEGIN {print int(factor * old_factor + 0.5)}')"
sed -i "$LINENUMBER s/^ \([0-9]\+\),$/ $NEW_FACTOR,/" "$PERFORMANCE_FILE"
)
done
Afterwards, we get comparable factors:
$ ./build/gobject/tests/performance/performance -v > p2
$ cat p2 | sed -n -e 's/^Running test //p' -e 's/.*correction factor //p' | sed 'N;s/\n/ /'
simple-construction 0.98
simple-construction1 0.75
complex-construction 0.99
complex-construction1 0.96
complex-construction2 1.02
finalization 1.05
type-check 0.98
emit-unhandled 1.01
emit-unhandled-empty 1.10
emit-unhandled-generic 1.03
emit-unhandled-generic-empty 1.07
...
Of course, this measurement was taken in my setup. But I think it
brings the base factors into a comparable range for most users.
Also, the commit message shows an ugly script how you can re-generate
this for your own purposes.
Move the factor inside the PerformanceTest structure, so it can be
programatically accessed.
More importantly, the number is now expressed directly beside the test
setup (the PerformanceTest structure), all at one place.
Also, each test now gets a separate factor.
This change will be useful in the next commit. So far there is no
notable change in behavior.
Despite assigning the function to a variable, gcc can still detect that
the function never changes and most of the test code is optimized out.
Initialize it somewhere, where the compiler cannot prove that this
function pointer is always set to the same value.
We could also make the pointer volatile, but this approach seems
preferable to me.
Cache the function pointer for g_datalist_id_update_atomic() in a static
variable in "gobject.c" to avoid looking it up repeatedly.
g_datalist_id_update_atomic() is anyway internal API. Like GData is not
a useful data structure in general, this function is only useful for
something specific inside GObject.
It can be easily seen that _local_g_datalist_id_update_atomic is never
read 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) is sufficient to ensure
thread-safe initialization. Note that we still set the pointer via
g_atomic_pointer_set(). This is done in an attempt to pacify thread
sanatizer.
Note that also with LTO enabled, the GLIB_PRIVATE_CALL() call cannot be
inlined. Previously we get:
0000000000011300 <_weak_ref_set>:
...
1131d: e8 ee 03 ff ff call 1710 <glib__private__@plt>
11322: 8b 35 0c b2 05 00 mov 0x5b20c(%rip),%esi # 6c534 <quark_weak_locations.lto_priv.0>
11328: 4c 89 e1 mov %r12,%rcx
1132b: 49 8d 7c 24 10 lea 0x10(%r12),%rdi
11330: 48 8d 15 b9 42 ff ff lea -0xbd47(%rip),%rdx # 55f0 <weak_ref_data_get_or_create_cb.lto_priv.0>
11337: ff 90 80 00 00 00 call *0x80(%rax)
afterwards:
0000000000011300 <_weak_ref_set>:
...
1131d: 48 8d 7e 10 lea 0x10(%rsi),%rdi
11321: 48 89 f1 mov %rsi,%rcx
11324: 48 8d 15 c5 42 ff ff lea -0xbd3b(%rip),%rdx # 55f0 <weak_ref_data_get_or_create_cb.lto_priv.0>
1132b: 8b 35 0b b2 05 00 mov 0x5b20b(%rip),%esi # 6c53c <quark_weak_locations.lto_priv.0>
11331: ff 15 f9 b1 05 00 call *0x5b1f9(%rip) # 6c530 <_local_g_datalist_id_update_atomic.lto_priv.0>
Also note, that the point here is not to optimize _weak_ref_set() (which
is not a hot path). There is work in progress that will use
g_datalist_id_update_atomic() for more purposes (and during more
relevant code paths of GObject).
During "as-installed" testing, we should search the GIR_DIR for GIR XML,
instead of hard-coding that it is `${prefix}/share/gir-1.0`. This is
not the case on at least Debian, in order to make it possible to
install more than one architecture's flavour of `GLib-2.0.gir`,
which contains some architecture-specific `#define`s.
Also search GOBJECT_INTROSPECTION_DATADIR/GIR_SUFFIX (in practice
something like `/usr/share/gir-1.0` in all cases) to accommodate
distributions like Debian that move the architecture-independent
majority of GIR XML into /usr/share to avoid duplication, leaving
only the architecture-specific minority of files like `GLib-2.0.gir`
in the GIR_DIR.
Signed-off-by: Simon McVittie <smcv@collabora.com>
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.
We were reusing the same logic everywhere, while we can just reuse an
unique class to base our tests on that avoids having to copy-and-paste
code for no good reason
Add some basic support for having glib-tests-only python libraries that
can be shared across the various projects, so that we don't have to
maintain multiple copies of them.
Move the shebang line from the full Python path of the build
machine to the first Python on the path during runtime. Set
the shebang in the main meson.build file so that it can be
overridden in one place if needed.
Fixes: #3331
We don't allow unloading types, both static and dynamic, since 2013. The
code that deals with reference counting is mostly dead code, and makes
reasoning about the type system more complicated than necessary.
Since type classes and interfaces can only be instantiated, we introduce
explicit getter functions that create a GTypeClass or a GTypeInterface
vtable; the ref() and unref() API gets a "soft" deprecation (explicitly
not using `GOBJECT_DEPRECATED_IN_*` yet), to allow people to
progressively port their code.
`@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
