Right now we create a bunch of GBytes which then get their reference count
incremented and immediately decremented. This causes quite a bit of
disruption for cacheline re-use.
Instead, this change creates an internal helper to transfer ownership of
GBytes to the new GVariant directly.
Surprisingly, this reduced wallclock time by about 6% for a contrived
benchmark of building "as" variant with GVariantBuilder.
When dealing with small allocations it can save considerable cycles to
do a single allocation for both the GBytes and the data by tacking it
onto the end of the GBytes.
Care is taken to preserve the glibc expectation of 2*sizeof(void*)
alignment of allocations at the expense of some padding bytes.
The degenerate case here is when you want to steal the bytes afterwards
but that amounts to the same overhead as the status-quo.
Where this can help considerably is in GVariant building such as
g_variant_new_int32() which allocates for the GVariant, the GBytes, and
the int32 within the GBytes.
In a simple benchmark of using GVariantBuilder to create "(ii)" variants
this saved about 10% in wallclock time.
In almost all cases, the variant is sunk shortly after creation and
certainly before other threads get a view of it.
This skips the bitlock for STATE_FLOATING, which is also done in
g_variant_take_ref(). We can do this because STATE_FLOATING may only
be set up-front, never after being cleared.
That allows us to do a relaxed read of the value once (which if it is
zero means no additional atomics beyond our ref count increment) as well
as a single atomic if we do in fact steal the floating reference.
Without any other GVariant performance changes, this is in the 2% range
of benchmarking a tight loop using GVariantBuilder. However, after the
rest of them are applied, the percentage is greater due to reduced
runtime overhead and lands in the 4.5% range.
If you have a definite-tuple type such as (iiii) then the number of
children that are allocated will match the offset when a GVariantBuilder
has completed.
That means we can avoid an expensive call into the allocator which is
normally done to shrink memory use by releasing it back to the allocator.
This saves about 5% of wallclock time when building such variants in a
tight loop.
This adds a fastpath for the extremely common case of checking if a
GVariant type is a subtype of itself _and_ a definite basic type. For
example, checking 'i' against 'i' or 's' against 's'.
In a loop using GVariantBuilder this can cut the cost of this function
alone in half on profiles from 3.3% of samples to 1.7% of samples.
We can get the length of the string if we provide an out argument to
g_utf8_validate(). This avoids an extra strlen() when creating GVariant
for UTF-8 strings.
This is good for nearly 7% reduction of CPU samples when building
heavily string-based GVariant using GVariantBuilder as a benchmark.
Any extremely common use-case of valid_format_string() is validation
when using GVariantBuilder. The optimal-case there is that there is
no programming error and thus the fast path should match.
This creates a fast path for that case without substantial change to the
GVariant type-checking case by checking for a non-NULL GVariant. It then
proceeds to hoist the actual scan directly without type allocation.
Locally this single change reduces wallclock time in a single-threaded
benchmark using GVariantBuilder by 17%.
Previously, g_variant_type_equal() would walk the strings multiple times.
In debug builds, you initially have the type checks for validity. But
also you walk the string to determine its length only to memcmp it after.
Instead, this does the comparison while walking the string for length.
Previously, G_VARIANT_TYPE() would "cast check" the type string unless
G_DISABLE_CHECKS was set. We very much don't want people to ever set
G_DISABLE_CHECKS but it is extremely normal to turn of cast-checks using
G_DISASABLE_CAST_CHECKS in stable releases.
This allows disabling the expensive type checking without catastrophically
disabling g_return_if_fail() macros.
Distributions are directed to disable G_ENABLE_DEBUG in stable builds.
Many do not currently disable assertion checks with G_DISABLE_ASSERT.
However this code is very much meant for debugging implementation details
rather than runtime validation. Use G_ENABLE_DEBUG instead of
!G_DISABLE_ASSERT to get the intended behavior.
I can’t see it being possible for this to be hit in practice, as it
would require two very long GVariant text format inputs, which would
probably hit input limits earlier on somewhere else.
But in order to avoid a silent integer overflow, let’s check that the
addition won’t overflow before going ahead with it.
Signed-off-by: Philip Withnall <pwithnall@gnome.org>
Helps: #3469
In `pattern_coalesce()`.
I am fairly sure this byte is never needed, as the most extreme case of
`max (strlen (left), strlen (right)` vs `strlen (left) + strlen (right)`
I can think of still gives 1 byte left in the buffer (without the need
for this commit).
However, the proof we have of how much space is needed in the buffer
only goes far enough to show that the number of bytes needed before the
nul terminator is at most `strlen (left) + strlen (right)`. That means
we still technically need to add an additional byte for the nul
terminator.
The need for this could be eliminated by coming up with a stronger proof
to show that the number of bytes needed is strictly less than `strlen
(left) + strlen (right)`, but I can’t do that, and adding one byte is
easy to do. Type strings are short and we’re nowhere near making a large
allocation here.
Signed-off-by: Philip Withnall <pwithnall@gnome.org>
Helps: #3469
The first of these tests is to cover the extreme case where the number
of bytes written out by `pattern_coalesce()` is as close to `strlen
(left) + strlen (right)` as we can manage, due to both inputs being
really small and hence `max (strlen (left), strlen (right))` being only
1 less than `strlen (left) + strlen (right)`, which is as close as we
can get to triggering an off-by-one error.
The second of these tests is an attempt to encode the case used in the
proof for correctness of `pattern_coalesce()` not overflowing its buffer
bounds: `(*(iii)) + ((iii)*) = ((iii)(iii))`. I don’t think it’s
actually possible to hit that case, as it’s not possible to generate a
plain `*` in either of the input patterns. `Ma*` is the best we can
manage in practice. See
https://gitlab.gnome.org/GNOME/glib/-/issues/3469#note_2226901 for
reasoning.
Signed-off-by: Philip Withnall <pwithnall@gnome.org>
Helps: #3469
This proof backs up the already-present assertion that “the length of
the output is loosely bounded by the sum of the input lengths”.
Signed-off-by: Philip Withnall <pwithnall@gnome.org>
Helps: #3469
This function is complicated and it's hard to assess whether it is
correct or not, so let's add an assert just to be sure.
Notably, when writing this I initially had an off-by-one error in my
assert, causing the assertion to actually fire when running the unit
tests. So we know it's definitely possible for the function to use the
entirety of the buffer. The upper bound is not loose, and writing one
additional byte would be a security bug.
Note my assertion possibly doesn't protect against security issues
because it will be hit after the bad write rather than before.
Fixes#3469
If the user passes -1 length to g_io_channel_set_line_term() along with
a nul-terminated string, then calls g_io_channel_get_line_term() and
decides to treat the result as nul-terminated rather than checking the
length parameter, then the application will have a problem, because it's
not nul-terminated. That's weird, since the input string was. Let's
ensure the result is consistent: if you pass a nul-terminated string,
the result is nul-terminated. If not, it's not.
Also add a warning to g_io_channel_get_line_term(), since it's very
strange for a gchar * return value to be anything other than a
nul-terminated UTF-8 string. This is an API design bug, but we cannot
fix it.
If %#Z is followed by %Z then we accidentally free the tmp variable from
the previous iteration of the loop a second time. Good job to the static
analysis tool (probably Coverity) that found this.
Fortunately it's unlikely that a realistic application would do this.
I've also added a new test that crashes without the fix
We leak if we have a non-null empty string. This was already fixed for
g_build_user_data_dir() in 1a979ab4947fc259af01ea65263aaa4d417553fb
so now fix the others as well.
I just spent several hours convinced that there was a memory safety
issue in string_parse() and bytestring_parse(). There isn't. (At least,
I think so.) Add some comments to save the next person some time.
Coverity has improperly flagged this code as an instance of CWE-562. The
code is fine, but it's confusing, and humans are likely to wind up
examining it again in the future, so add some comments to explain what's
up.
The value returned when generating a collation key is an opaque binary
blob that is only meant to be used for byte-wise comparisons; we should
not imply it's a nul-terminated, UTF-8 string.
This is especially true for language bindings that try to convert C
strings returned by GLib into UTF-8 encoded strings.
Ideally, the collation functions should return a byte array, but the
closest thing we have is the OS native encoding type that we use for
paths and environment variables.
See: https://github.com/gtk-rs/gtk-rs-core/issues/1504
When the child process is going to exit on error after fork() but before
exec(), let's close the child_err_report_fd. The practical value of this
is to placate valgrind --track-fds=yes.
The changes made in commit bc59e28bf6b0f70ff345aef80356d0076f44a0e7
(issue #3399) fixed introspection of the GThread API. However, they
introduced a trampoline in every threading function. So with those
changes applied, the disassembly of `g_mutex_lock()` (for example) was:
```
0x7ffff7f038b0 <g_mutex_lock> jmp 0x7ffff7f2f440 <g_mutex_lock_impl>
0x7ffff7f038b5 data16 cs nopw 0x0(%rax,%rax,1)
```
i.e. It jumps straight to the `_impl` function, even with an optimised
build. Since `g_mutex_lock()` (and various other GThread functions) are
frequently run hot paths, this additional `jmp` to a function which has
ended up in a different code page is a slowdown which we’d rather avoid.
So, this commit reworks things to define all the `_impl` functions as
`G_ALWAYS_INLINE static inline` (which typically expands to
`__attribute__((__always_inline__)) static inline`), and to move them
into the same compilation unit as `gthread.c` so that they can be
inlined without the need for link-time optimisation to be enabled.
It makes the code a little less readable, but not much worse than what
commit bc59e28bf6b0f70ff345aef80356d0076f44a0e7 already did. And perhaps
the addition of the `inline` decorations to all the `_impl` functions
will make it a bit clearer what their intended purpose is
(platform-specific implementations).
After applying this commit, the disassembly of `g_mutex_lock()`
successfully contains the inlining for me:
```
=> 0x00007ffff7f03d80 <+0>: xor %eax,%eax
0x00007ffff7f03d82 <+2>: mov $0x1,%edx
0x00007ffff7f03d87 <+7>: lock cmpxchg %edx,(%rdi)
0x00007ffff7f03d8b <+11>: jne 0x7ffff7f03d8e <g_mutex_lock+14>
0x00007ffff7f03d8d <+13>: ret
0x00007ffff7f03d8e <+14>: jmp 0x7ffff7f03610 <g_mutex_lock_slowpath>
```
I considered making a similar change to the other APIs touched in #3399
(GContentType, GAppInfo, GSpawn), but they are all much less performance
critical, so it’s probably not worth making their code more complex for
that sake.
Signed-off-by: Philip Withnall <pwithnall@gnome.org>
Fixes: #3417
And size it to `sysconf (_SC_MINSIGSTKSZ)`, if defined.
This might fix the sigaltstack test on muslc, as suggested by Markus
Wichmann (https://www.openwall.com/lists/musl/2024/05/30/2) and Rich Felker
(https://www.openwall.com/lists/musl/2024/05/29/7) on the musl mailing
list.
The thinking is that the CI machine might have a huge register file
(AVX512 or some other large extension) which doesn’t fit in `MINSIGSTKSZ`.
By sizing the alternate stack to `sysconf (_SC_MINSIGSTKSZ)` we should
be able to avoid that.
Moving it onto the heap should avoid any potential complications or bugs
from having one stack embedded within another.
Signed-off-by: Philip Withnall <pwithnall@gnome.org>
Fixes: #3315
This function type isn't only used by g_hash_table_foreach_remove(), and
what happens to the data when we return TRUE depends on the calling
function.
Includes a port to modern gi-docgen syntax by Emmanuele Bassi.
Signed-off-by: Adrien Plazas <adrien.plazas@codethink.co.uk>
