(In reply to Miguel París Díaz from comment #0)
> To fix this I think that one approach is:
>   - If the user is not interested in the error, it must no be managed.

Right... this could be fixed by adding a g_set_error() wrapper macro around g_set_error() (and probably likewise g_set_error_literal()) that would check if error was NULL before evaluating its arguments.

Though at least theoretically, that is an ABI break, since code could potentially depend on the side effects of g_set_error() being executed...

>   - If the user is interested in the error:
>     - If he is only interested in the error type, the error string must no
> be processed

Unfortunately we have no way of knowing that this is the case from the point where we have to create the GError

>     - If he is interested in the error string (2 alternatives):
>        1) The string should be processed internally.

It looks like a simple improvement here would be to change g_strerror() to keep its own errno->utf8-string hash table rather than using g_intern_string(); then it would only make the expensive g_locale_to_utf8() call once rather than making it every time it was called.

Created attachment 308947 [details] [review]
Make g_strerror() do less work

Remember the UTF-8-encoded error strings so that people in non-UTF-8
locales don't need to call g_locale_to_utf8() in every g_strerror()
call.

==========

Does this make your callgrind look better?

Created attachment 309918 [details]
Callgrind graph (g_strerror do less work)

Created attachment 309919 [details]
Callgrind graph (g_strerror do less work)

First at all, sorry for the delayed response.

I have profiled your patch and it improves the performance 3x :D (you can see the new callgrind graph).
From this graph we can see that now g_set_error and glib_gettext are the most expensive functions.
Could we do something to avoid it or reduce their cost?

Created attachment 309920 [details]
g_socket_send_message - Callgrind graph

I have also profiled g_socket_send_message (see callgrind graph), and I see some possible improvements:
  - Could we keep the native address info into GSocketAddress and only update it if it changes instead of generating it per each send_message call?

Source code:
g_socket_send_message
  https://git.gnome.org/browse/glib/tree/gio/gsocket.c?id=2.42.2#n3739
Getting native address:
  https://git.gnome.org/browse/glib/tree/gio/gsocket.c?id=2.42.2#n3802

Can you attach the full (non-graphical) profiling output? (eg, to see what part of g_error_new_valist() is so expensive)

Created attachment 309925 [details]
Callgrind graph (g_strerror do less work)

It is huge, so I have attached the complete graph.
I hope it help you ;).

It might be worth making an API call that doesn't malloc at all in this case.  Something like an extra `gboolean *out_would_block` parameter?

GLib is a pretty malloc-happy library in general, and that's hard to avoid, but I think we could do better with a few targeted APIs for inner loops.

Created attachment 309969 [details] [review]
gsocket: add a wrapper around g_set_error() to avoid extra work

If @error is NULL then we don't even need to evaluate the remaining
arguments. And if errno is EWOULDBLOCK, then no one should see the
error message anyway, so don't bother g_strdup_printf'ing up a pretty
one.

Review of attachment 309969 [details] [review]:

This is something that we should consider promoting to GError API if more things use it.

Another case I had is that it's very common to check for file existence in multiple places,
and formatting a whole GError only to look for the equivalent of ENOENT was expensive.
(This is in OSTree, and I mostly dropped GIO for local files for this and other reasons
 like being able to use openat() etc.)

::: gio/gsocket.c
@@ +246,3 @@
 
+/* Wrapper around g_set_error() to avoid doing excess work */
+#define socket_set_error(err, domain, code, fmt, strerr)                \

Maybe `socket_set_error_lazy` to make clearer what it does?

And as long as you're making this a macro, might as well
suck in the common G_IO_ERROR, socket_io_error_from_errno (errsv)
and strerror arguments.

i.e.
socket_set_error_lazy (error, errsv,  _("Error accepting connection: %s"))
?

Review of attachment 308947 [details] [review]:

I am not super excited about this.  Yes, it's better but it's still a hack.

We need to be more aggressive about driving change into glibc, the Linux kernel etc.
where it makes things better.

In this case, "strerror_in_utf8()" seems like quite a sane thing to ask for.  In
fact there is already `strerror_l` - can we just give that a UTF-8 variant?

Hmm...actually, I'd be happy if we just preserved the if (g_get_charset()) fast
path for the sane case where the locale is already UTF-8.  In that case the old
code was wrong in interning the string - we should be able to just reuse the value
of strerror() directly.

(Okay except for threadsafety/lifetime issues - by interning it we were making it
 threadsafe and live forever, where as sterror() is not threadsafe...ugh)

Review of attachment 308947 [details] [review]:

::: glib/gstrfuncs.c
@@ +1247,3 @@
   gint saved_errno = errno;
 
+  G_LOCK (errors);

I worry about locking too much. I would do this solution to avoid it and it is also more efficient that using a hash table:

static gchar * errors[N]; /* We should know how many errors there are
                             or use a enough number */

ret = g_atomic_pointer_get (errors[ernum]);
if (ret == NULL) { /* Only access to the critical section when there is not value */
  G_LOCK (errors);

  ret = g_atomic_pointer_get (errors[ernum]);
  if (ret == NULL) { /* Check again into critical section */
    ret = strerror (errnum);
    if (!g_get_charset (NULL)) {
      ret = g_locale_to_utf8 (ret, -1, NULL, NULL, NULL);
    }
    g_atomic_pointer_set (errors[errnum], ret);
  }

  G_UNLOCK (errors);
}

(In reply to Colin Walters from comment #13)
> This is something that we should consider promoting to GError API if more
> things use it.

I thought about wrapping g_set_error(), etc in macros that would keep their arguments from being evaluated if error was NULL. But it's technically an ABI break; someone could be relying on a side effect occurring in one of those arguments. (GLib itself still passes "make check" with this change though.)

(Option 2: make that behavior opt-in via a #define.)

I also figured it probably wouldn't be worth it, since you're probably only going to have calls to gettext() in your g_set_error() args, and I'd expected that those would be reasonably well optimized (though maybe they aren't...)

> Another case I had is that it's very common to check for file existence in
> multiple places,
> and formatting a whole GError only to look for the equivalent of ENOENT was
> expensive.

I had a thought that it might be nice to be able to create "static" GErrors, where passing them to g_error_free() is a no-op. Then code could just keep some stock GErrors around rather than having to make a new one each time. (This would require some way of "extending" GError, which bug 684969 talks about.)

That probably wouldn't work for ENOENT though, since you'd need to include the filename in the message.


> And as long as you're making this a macro, might as well
> suck in the common G_IO_ERROR, socket_io_error_from_errno (errsv)
> and strerror arguments.

oops, yeah. Originally it was a varargs macro and so I needed to be able to do "#define socket_set_error g_set_error" in the !G_HAVE_ISO_VARARGS case. But then I noticed that all the places I cared about had the same args anyway.

If we can solve the immediate bug here with just having gsocket.c be lazy, I'd vote for going with that.

(In reply to Colin Walters from comment #14)
> In this case, "strerror_in_utf8()" seems like quite a sane thing to ask for.
> In
> fact there is already `strerror_l` - can we just give that a UTF-8 variant?

In this case the amount of necessary extra code (mangling LC_MESSAGES/LC_ALL/LANG, calling newlocale(), extra configure checks and #ifdefs) vs added efficiency (getting the right translation directly rather than having to transcode it *once*) seems not really worth it, particularly since we need to keep the non-strerror_l() version as well anyway.

> Hmm...actually, I'd be happy if we just preserved the if (g_get_charset())
> fast
> path for the sane case where the locale is already UTF-8.  In that case the
> old
> code was wrong in interning the string - we should be able to just reuse the
> value
> of strerror() directly.
> 
> (Okay except for threadsafety/lifetime issues - by interning it we were
> making it
>  threadsafe and live forever, where as sterror() is not threadsafe...ugh)

Yeah. Although in theory the code is already wrong/unsafe; someone could call strerror() in another thread in between when we call it and when we call g_intern_string(). (Or while g_intern_string() is waiting for its mutex, etc.) Though really I don't know why it doesn't just say that the behavior is undefined if @errnum isn't a valid errno value... (And if we then assume that libc returns a static string if @errnum is valid, which is totally reasonable, then the problem is solved.)

Comment on attachment 309969 [details] [review]
gsocket: add a wrapper around g_set_error() to avoid extra work

Pushed the socket_set_error_lazy() patch with Colin's suggested changes.

Attachment 309969 [details] pushed as 1ab3e3e - gsocket: add a wrapper around g_set_error() to avoid extra work

Great ;).
What about my comments in patch https://bugzilla.gnome.org/review?bug=752769&attachment=308947 ?

(In reply to Miguel París Díaz from comment #20)
> Great ;).
> What about my comments in patch
> https://bugzilla.gnome.org/review?bug=752769&attachment=308947 ?

If you want to pursue it let's make a separate bug.

I think it's useful to keep this bug open, as it's about optimizing a specific use case, even though that optimization is going to require changes in a handful of different places

I agree with Dan,
moreover the most impact patch is under discussion yet.

Comment on attachment 308947 [details] [review]
Make g_strerror() do less work

an updated version of this was committed as part of bug 754788

Created attachment 311283 [details] [review]
gsocket: use the "recv_addr_cache" for send addresses too

======

We're already caching GSocketAddress<->struct sockaddr mappings for
g_socket_receive_message(), so let's try using that cache for
g_socket_send_message() too...

-- GitLab Migration Automatic Message --

This bug has been migrated to GNOME's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/glib/issues/1064.