As per the previous change, an object that had weak locations set may
need to lock again the weak locations mutex during qdata cleanup, but
we can avoid this when we know we're removing the last location, by
removing the qdata entry and freeing the data.
In case a new location is needed for the same object, new data will be
added.
However, by doing this the weak locations during dispose may be
invalidated once the weak locations lock is passed, so check again if
this is the case while removing them.
It can happen that a GWeakRef is added to an object while it's disposing
(or even during finalizing) and this may happen in a thread that (weak)
references an object while the disposal isn't completed yet or when
using toggle references and switching to GWeakRef on notification (as
the API suggests).
In such scenario the weak locations are not cleaned up when the object
is finalized, and will point to a free'd area.
So, during finalization and when we're sure that the object will be
destroyed for sure, check again if there are new weak locations and
unset them if any as part of the qdata destruction.
Do this adding a new utility function so that we can avoid duplicating
code to free the weak locations.
Added various tests simulating this case.
Fixes: #2390
The documentation sort of already said this, but it’s better to make it
explicit.
This avoids the situation where some of the weak notify callbacks for an
object have been called, and then a subsequent one resurrects the
object. Without some way of undoing the weak notifications already sent,
that would leave external state which is coupled to the object’s
lifecycle out of sync.
This arose from discussion on !2064.
Signed-off-by: Philip Withnall <pwithnall@endlessos.org>
GTK currently checks if a GtkWidget is finalized while still using a
floating reference—i.e. a widget was disposed without any parent
container owning it.
This warning can be useful to identify and trace ownership transfer
issues in libraries using initially unowned floating object types.
To avoid introducing constraints ex post, we can gate this check behind
both the G_ENABLE_DEBUG compile time flag for GLib, and behind the
G_ENABLE_DIAGNOSTIC environment variable run time check.
Fixes: #2489
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
Instead of calling xterm when it clearly does not exist and causes a silent error,
inform the user that the launch failed so they can take the right action.
Also added (transfer full) or (transfer none) annotations while I was at it.
This is the result of checking each `Returns:` line in these files. I’ve
only considered nullability and transferability, and not other (potentially
missing or incorrect) annotations.
Helps: #2227
If the first power-profile installed test fails (for example, because
xdg-desktop-portal isn’t available), correctly tear down the dbusmock
object, or it will cause setUp() to fail when the next test in the suite
is run.
Signed-off-by: Philip Withnall <pwithnall@endlessos.org>
Helps: #2481
When first creating the monitor, correctly set its property value to the
value from the portal, rather than waiting for the portal value to
change to set it.
Signed-off-by: Philip Withnall <pwithnall@endlessos.org>
Fixes: #2481
We were lucky that this worked in some cases (the test is racy), but we
should actually run the condition check each loop, rather than when the
function is called.
Spotted by Martin Pitt:
96a8c02d24 (r54773831)
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
Now that `g_thread_pool_new_full()` can be used to set a user-provided
free function for queue elements, ensure that the internal dummy item
used to wake up the worker threads is removed from the queue before it’s
called.
Signed-off-by: Philip Withnall <pwithnall@endlessos.org>
Helps: #2456
