It fixes a race. g_source_attach() had the following check to ensure a
loop blocked on poll() would wakeup.
if (do_wakeup && context->owner && context->owner != G_THREAD_SELF)
g_wakeup_signal (context->wakeup);
However it doesn't contemplate an implementation where poll()ing is a
non-blocking operation that will be scheduled while the thread is
released to perform other tasks. This scenario opens up several
different possibilities where the condition would fail to hold true. I
experienced two of such races.
The first race pertains to a mono-threaded application. Do keep in mind
that integrating GLib to a foreign event loop will make GLib act as a
slave in the new event loop. When you post a new work unit to execute in
the thread managed by the foreign event loop, you don't use
g_main_context_invoke(). In fact the only reason to integrate
GMainContext in a foreign event loop is to make the two of them
communicate. So from time to time, the foreign event loop will execute
callbacks that manipulate the GMainContext loop. An illustration
follows.
// in this callback we translate an event from the foreign event loop
// to an event in the GMainContext event loop (that runs in the same
// thread)
static void my_event_loop_callback(void* data)
{
GMainContext* ctx = /* ... */;
// ...
g_source_attach(source, ctx);
}
int main()
{
// ...
my_event_loop_invoke(my_event_loop_callback, data);
// ...
// this function has all mechanisms in place to run the foreign
// event loop and the hooks to call
// g_main_context_{prepare,query,check,dispatch}
my_event_loop_run();
}
In this case, you would have the following series of calls:
1. g_main_context_prepare()
2. g_main_context_query()
3. A callback to my_event_loop is registered when any fd on the set is
ready or the timeout is reached.
4. The thread is released to perform other tasks.
5. One of the tasks executed wishes to communicate with my_event_loop
and enters my_event_loop_callback.
6. g_source_attach() is called.
7. g_source_attach() detects do_wakeup=TRUE, context->owner != NULL, and
context->owner == G_THREAD_SELF so g_wakeup_signal() is skipped.
8. None of the fds on the GLib poll() set becomes ready nor the GLib
timeout expires. The my_event_loop callback that would call
g_main_context_check() is never executed. Deadlock.
A shallow analysis will fail to detect the race here. The explanation
seems to showcase a scenario that will deterministically fail with a
deadlock every time. However do keep in mind that my_event_loop_callback
could be invoked before or after g_main_context_prepare(). There is an
_event_ race here. Furthermore, some GLib libraries such as GDBus will
initialize objects from extra threads (GAsyncInitable interface) and
invoke the result on the original thread when ready (g_source_attach()
will eventually be called). Now you have scenarios closer to standard
race examples.
The other scenario where a race would manifest happens in a
multi-threaded application that has a concurrency design similar to the
actor model. No actor executes in two threads simultaneously, but it's
not guaranteed that it'll always wake-up in the same thread. It'd
perform steps 1-4 just as in the previous example, but before thread
control is returned to the pool, it'd call g_main_context_release(). Now
g_source_attach() would skip g_wakeup_signal() for a different reason:
7. g_source_attach() detects do_wakeup=TRUE, context->owner == NULL so
g_wakeup_signal() is skipped.
8. Same as before.
Certainly there are other concurrency designs where this optimization
would cause a deadlock, but all of them have origin in the same place:
the optimization assumes the poll() implementation is a blocking
operation and the thread will never be released to perform other tasks
(possibly involving GLib calls) while result is not ready. They share
not only the same problem, but also the same solution: do not make
assumptions and just call g_wakeup_signal().
This patch implements this solution by introducing the
G_MAIN_CONTEXT_FLAGS_OWNERLESS_POLLING flag. This flag will force a call
to g_wakeup_signal() and fix the race on foreign event loops. The reason
to prevent changing this option after creation is to avoid other races
that would lead to event loss. Construction is the only proper time to
set this option.
The implementation design means we do not change **any** semantics for
current working code. If you don't set the new flag, the code won't
enter in different branches and current behavior won't be affected. The
patch is small and easy to follow too.
Previously, priority was not randomly generated and was instead derived
from `GSequenceNode*` pointer value.
As a result, when a `GSequence` was freed and another was created, the
nodes were returned to memory allocator in such order that allocating
them again caused various performance problems in treap.
To my understanding, the problem develops like this :
1) Initially, memory allocator makes some nodes
2) For each node, priority is derived from pointer alone.
Due to the hash function, initially the priorities are reasonably
randomly distributed.
3) `GSequence` moves inserted nodes around to satisfy treap property.
The priority for node must be >= than priorities of its children
4) When `GSequence` is freed, it frees nodes in a new order.
It finds root node and then recursively frees left/right children.
Due to (3), hashes of freed nodes become partially ordered.
Note that this doesn't depend on choice of hash function.
5) Memory allocator will typically add freed chunks to free list.
This means that it will reallocate nodes in same or inverse order.
6) This results in order of hashes being more and more non-random.
7) This order happens to be increasingly anti-optimal.
That is, `GSequence` needs more `node_rotate` to maintain treap.
This also causes the tree to become more and more unbalanced.
The problem becomes worse with each iteration.
The solution is to use additional noise to maintain reasonable
randomness. This prevents "poisoning" the memory allocator.
On top of that, this patch somehow decreases average tree's height,
which is good because it speeds up various operations. I can't quite
explain why the height decreases with new code, probably the properties
of old hash function didn't quite match the needs of treap?
My averaged results for tree height with different sequence lengths:
Items | before| after |
--------+-------+---------------+
2 | 2,69 | 2,67 -00,74% |
4 | 3,71 | 3,80 +02,43% |
8 | 5,30 | 5,34 +00,75% |
16 | 7,45 | 7,22 -03,09% |
32 | 10,05 | 9,38 -06,67% |
64 | 12,97 | 11,72 -09,64% |
128 | 16,01 | 14,20 -11,31% |
256 | 19,11 | 16,77 -12,24% |
512 | 22,03 | 19,39 -11,98% |
1024 | 25,29 | 22,03 -12,89% |
2048 | 28,43 | 24,82 -12,70% |
4096 | 31,11 | 27,52 -11,54% |
8192 | 34,31 | 30,30 -11,69% |
16384 | 37,40 | 32,81 -12,27% |
32768 | 40,40 | 35,84 -11,29% |
65536 | 43,00 | 38,24 -11,07% |
131072 | 45,50 | 40,83 -10,26% |
262144 | 48,40 | 43,00 -11,16% |
524288 | 52,40 | 46,80 -10,69% |
The memory cost of the patch is zero on 64-bit, because the new field
uses the alignment hole between two other fields.
Note: priorities can sometimes have collisions. This is fine, because
treap allows equal priorities, but these will gradually decrease
performance. The hash function that was used previously has just one
collision on 0xbfff7fff in 32-bit space, but such pointer will not
occur because `g_slice_alloc()` always aligns to sizeof(void*).
However, in 64-bit space the old hash function had collisions anyway,
because it only uses lower 32 bits of pointer.
Closes#2468
The previous test was racy: it assumed that not all queued thread pool
jobs would start to be executed before `g_thread_pool_free()` was
called, whereas actually on some systems (particularly BSD ones), they
would all start (or even finish) being executed, and hence the free
function might never be called.
Rewrite the test to:
• Synchronise the test function and worker thread functions more
closely.
• Not bother about ordering the shared and exclusive variants of the
test differently. That seems to be a hangover from another test
above.
• Limit the number of worker threads to 1, rather than 2, since this
makes the test easier to control.
This has been tested with `--repeat 10000` on Linux, and it succeeds all
of those runs whereas previously it failed quite reliably.
Signed-off-by: Philip Withnall <pwithnall@endlessos.org>
Fixes: #2456
This allows a pattern like
g_test_message ("cannot reticulate splines: %s", error->message);
g_test_fail ();
to be replaced by the simpler
g_test_fail_printf ("cannot reticulate splines: %s", error->message);
with the secondary benefit of making the message available to TAP
consumers as part of the "not ok" message.
Signed-off-by: Simon McVittie <smcv@collabora.com>
Forming the g_test_skip() message from printf-style arguments seems
common enough to deserve a convenience function.
g_test_incomplete() is mechanically almost equivalent to g_test_skip()
(the semantics are different but the implementation is very similar),
so give it a similar mechanism for symmetry.
Signed-off-by: Simon McVittie <smcv@collabora.com>
We had two compensating bugs here. We didn't correctly clear the
error indicator after testing a test-case that calls g_test_fail(),
which meant we were leaving the error set to exit status 1 when
falling through to the next test; and then we didn't check the exit
status of the next test, but instead assumed that g_spawn_sync()
would fail (it does not).
Signed-off-by: Simon McVittie <smcv@collabora.com>
Previously, these would have done 2**32 replacements, and the first one
would have consumed 6GB of memory in the process. They now match what
Python `str.replace()` does.
Reproduces: https://gitlab.gnome.org/GNOME/glib/-/issues/2452
Signed-off-by: Simon McVittie <smcv@collabora.com>
These are taken from another project (steam-runtime-tools) where I
implemented a similar replace method before realising that more recent
GLib versions had g_string_replace().
Signed-off-by: Simon McVittie <smcv@collabora.com>
g_source_set_name duplicates the string, and this is
showing up as one of the more prominent sources of strdups
in GTK profiles, despite all the names we use being literals.
Add a variant that avoids the overhead.
This should maintain equivalent functionality, apart from that now you
have to pass `--force-fallback-for libpcre` to `meson configure` in
order to use the subproject; rather than specifying
`-Dinternal_pcre=true` to use the internal copy.
This also fixes#642, as the wrapdb copy of libpcre is version 8.37.
Signed-off-by: Philip Withnall <pwithnall@endlessos.org>
Helps: #962Fixes: #642
This is basically a contrived test to trigger the `bytes->user_data !=
bytes->data` condition (and none of the earlier short-circuiting
conditions in that statement) in `try_steal_and_unref()`. This gives
100% line and branch coverage for `gbytes.c`.
Signed-off-by: Philip Withnall <pwithnall@endlessos.org>
On Unix platforms, wait() and friends yield an integer that encodes
how the process exited. Confusingly, this is usually not the same as
the integer passed to exit() or returned from main(): conceptually it's
an integer encoding of this tagged union:
enum { EXITED, SIGNALLED, ... } tag;
union {
int exit_status; /* if EXITED */
struct {
int terminating_signal;
bool core_dumped;
} terminating_signal; /* if SIGNALLED */
...
} detail;
Meanwhile, on Windows, wait statuses and exit statuses are
interchangeable.
I find that it's clearer what is going on if we are consistent about
referring to the result of wait() as a "wait status", and the value
passed to exit() as an "exit status".
GSubprocess already gets this right: g_subprocess_get_status() returns
the wait status, while g_subprocess_get_exit_status() genuinely returns
the exit status. However, the GSpawn family of APIs has tended to
conflate the two.
Confusingly, g_spawn_check_exit_status() has always checked a wait
status, and it would not be correct to pass an exit status to it; so
let's deprecate it in favour of g_spawn_check_wait_status(), which
does the same thing that g_spawn_check_exit_status() always did.
Code that needs backwards-compatibility with older GLib can use:
#if !GLIB_CHECK_VERSION(2, 69, 0)
#define g_spawn_check_wait_status(x) (g_spawn_check_exit_status (x))
#endif
Signed-off-by: Simon McVittie <smcv@collabora.com>
If there were more subpatterns in the regex than matches (which can
happen if one or more of the subpatterns are optional),
`g_match_info_fetch()` was erroneously returning `NULL` rather than the
empty string. It should only return `NULL` when the `match_num`
specifies a subpattern which doesn’t exist in the regex.
This is complicated slightly by the fact that when using
`g_regex_match_all()`, more matches can be returned than there are
subpatterns, due to one or more subpatterns matching multiple times at
different offsets in the string.
This includes a fix for a unit test which was erroneously checking the
broken behaviour.
Thanks to Allison Karlitskaya for the minimal reproducer.
Signed-off-by: Philip Withnall <pwithnall@endlessos.org>
Fixes: #229
Rather than predicating the test on whether the system PCRE is being
used, use a more specific version comparison which should work
regardless of whether the system or internal copy of libpcre is being
used.
Signed-off-by: Philip Withnall <pwithnall@endlessos.org>
Helps: #962
Also move env setup earlier in the test, to ensure that
the child gets the envvars during initialization.
Also, don't look for exception codes in stderr, since
OutputDebugStringA() doesn't dump stuff there.
