GNOME Bugzilla – Bug 754788
more g_strerror stuff
Last modified: 2015-09-11 16:45:26 UTC
Spinoff of bug 752769, where it was determined that g_strerror()'s use of g_locale_to_utf8() (in non-UTF-8 locales) is a major performance hit in some places. Someone had raised the objection there that adding a G_LOCK here might have its own performance issues, but it was already locking a global mutex before anyway, inside g_intern_string(), so this just moves the mutex somewhere else. (Alternatively, we could assume that if errnum < sys_nerr, then "obviously" strerror() will return a constant static pointer and we don't need to do any caching ourselves...)
Created attachment 311005 [details] [review] More g_strerror() fixes Add a check to configure.ac for strerror_r, since we don't currently require POSIX.1-2001 conformance in general. Add back a plain-strerror() case as a fallback, and rearrange the glibc-vs-POSIX strerror_r() branches. Update the docs to not claim that "not all platforms support the strerror() function" (we require C90), but still mention the UTF-8 and always-valid-string benefits. (And make test_strerror() check that last part.)
Created attachment 311006 [details] [review] Make g_strerror() do less work Store the (translated, UTF-8-encoded) error strings in a hash table so to avoid doing translation and (possibly) g_locale_to_utf8() in every g_strerror() call.
Review of attachment 311005 [details] [review]: Just one comment. ::: glib/gstrfuncs.c @@ -1265,3 @@ - * expect to get the GNU variant of strerror_r. However, use the - * provided check from man strerror_r(3) in case we ever stop using - * _GNU_SOURCE (admittedly unlikely). I thought this was a kind of useful comment, why delete it? If it's too verbose we could just say: /* Match the conditional in strerror(3) for glibc */
Review of attachment 311006 [details] [review]: I don't object to this. But as discussed we know at least with glibc and UTF-8 (a very common case), the returned strings are static if the error number is valid. And actually glibc exports this as a symbol "sys_nerr" (though it's deprecated, it's going to be around forever). Hmm, we still have the lifetime issues in that g_strerror() is bound to live forever. You know what we really want is what glibc already has: the %m specifier for g_set_error().
(In reply to Colin Walters from comment #4) > I don't object to this. But as discussed we know at least with glibc and > UTF-8 (a very common case), the returned strings are static if the error > number is valid. And actually glibc exports this as a symbol "sys_nerr" Yeah, but strerror_r() always calls _(), which looks like it's probably more expensive than G_LOCK()+g_hash_table_lookup()+G_UNLOCK(), so I think this version is better anyway (on the second and later calls for a given errnum). > Hmm, we still have the lifetime issues in that g_strerror() is bound to live > forever. what do you mean?
(In reply to Dan Winship from comment #5) > > > Hmm, we still have the lifetime issues in that g_strerror() is bound to live > > forever. > > what do you mean? strerror() isn't threadsafe, hence the introduction of strerror_r(). Except we haven't documented the threadsafety of g_strerror() - but clearly we have been at least *trying* to make it thread safe before, and realistically should continue to do so (right?). The only way to make a const char * return value threadsafe is to have it "live forever", i.e. "&'static str" in Rust terms, right?
(In reply to Colin Walters from comment #6) > strerror() isn't threadsafe, hence the introduction of strerror_r(). Except > we haven't documented the threadsafety of g_strerror() - but clearly we have > been at least *trying* to make it thread safe before, and realistically > should continue to do so (right?). Right. I added that to the docs in my patch: + * such process". Unlike strerror(), this always returns a string in + * UTF-8 encoding, and the pointer is guaranteed to remain valid for + * the lifetime of the process. > The only way to make a const char * return value threadsafe is to have it > "live forever", i.e. "&'static str" in Rust terms, right? Right, hence the static GHashTable which never gets freed or cleaned up.
The "issue" I was referring to is that we can't have a thin wrapper for what glibc is doing, we'd have to introduce g_strerror_r(). Hmm. On glib+glibc, we know g_printf = printf which supports %m. We could have: g_set_error_with_strerror() which on glibc would use %m, on other Unix would do g_set_error(..., "foo: %s", g_strerror()) ?
(In reply to Colin Walters from comment #8) > The "issue" I was referring to is that we can't have a thin wrapper for what > glibc is doing, we'd have to introduce g_strerror_r(). Huh? Are you saying that there is some bug in the current version of the patch attached here (other than the unavoidable race condition on systems that don't have strerror_r())? If so, what exactly? I claim that with this patch, on glibc (and Windows, and anything implementing POSIX strerror_r()), there are no race conditions or thread-safety issues, and the return value of g_strerror() is guaranteed to accurately reflect the passed-in errnum, and be valid in all threads, for the lifetime of the program, even if g_strerror() is also called simultaneously in another thread with a different errnum, etc.
I don't see any problems with your patch, please commit. I'm just trying to think about how things could be *more* optimized for the common case where we're doing g_set_error (err, ..., "Failed to blah: %s", g_strerror (errno)); In that case the glibc %m specifier I believe would be more efficient as we wouldn't need the separate string when we're just going to copy it onto the end of another/