Faster manager_process_barrier_fd and drop message if BARRIER=1 found #15660
Conversation
benjarobin
force-pushed
the
perf_barrier_fd
branch
from
eb5f2cc
to
7a5b476
Compare
poettering
added
needs-discussion 🤔
reviewed/needs-rework 🔨
Indicates that the tags list cannot be modified by notify_message function. Since the tags list is created only once for multiple call to notify_message functions.
benjarobin
force-pushed
the
perf_barrier_fd
branch
from
7a5b476
to
7bff205
Compare
|
@poettering This is a complete rewrite of the previous MR. I hope you will like the idea. |
| @@ -106,7 +106,7 @@ bool strv_overlap(char * const *a, char * const *b) _pure_; | |||
|
|
|||
| #define STRV_FOREACH_BACKWARDS(s, l) \ | |||
| for (s = ({ \ | |||
| char **_l = l; \ | |||
| typeof(l) _l = l; \ | |||
| @@ -3894,19 +3894,19 @@ static void service_force_watchdog(Service *s) { | |||
| static void service_notify_message( | |||
| Unit *u, | |||
| const struct ucred *ucred, | |||
| char **tags, | |||
| char * const *tags, | |||
There was a problem hiding this comment.
we mostly gave up on using const on arrays of strings, since the semantics of that are so fucked in C. but i guess it doesn't hurt if it works here
There was a problem hiding this comment.
I have tried to had a few const at some place, I quickly forget about it. And yes I am agree this part in the C language is "not perfect".
|
|
||
| buf = strv_join(tags, ", "); |
There was a problem hiding this comment.
needs oom check. i mean, if we actually hit oom it might be fine to ignore it, given this is about debug logging only, but we shouldn't pass NULL to ellipsize() in that case
There was a problem hiding this comment.
I choose to do the same things that with the output of ellipsize, ignore it.
| if (fdset_size(fds) != 1) | ||
| log_warning("Got incorrect number of fds with BARRIER=1, closing them."); | ||
| if (strv_contains(tags, "BARRIER=1")) { | ||
| if (strv_length(tags) == 1) { |
There was a problem hiding this comment.
you iterate through the array twice here. maybe:
if (strv_equal(tags, STRV_MAKE("BARRIER=1")) {
…
} else if (strv_contains(tags, "BARRIER=1")) {
…
}that way you iterate through the array only once.
There was a problem hiding this comment.
Since the reception of "BARRIER=1" is less frequent (currently nobody uses it, but statically, it will always be less frequent), we can write it in the reverse way:
if (strv_contains(tags, "BARRIER=1")) {
if (strv_equal(tags, STRV_MAKE("BARRIER=1")) {
But technically strv_length is very cheap, and faster than strv_equal() (Just iterating on a array costs less than a string compare).
So even if there is a slight improvement, I do not think that it is easier to read.
There was a problem hiding this comment.
If somebody really care about performance, (strv_length(tags) == 1) can be replaced (in this specific case) by (tags[1] == NULL)
But I don't think this is necessary
There was a problem hiding this comment.
strv_equal() is O(1) time in this case where the second argument is literal, it's hard to beat that.
|
i very much like the idea of converting this earlier into an an array of strings, btw! |
- Parse the tags list using strv_split_newlines() which remove any unnecessary empty string at the end of the strv. - Use this parsed list for manager_process_barrier_fd() and every call to manager_invoke_notify_message(). - This also allow to simplify the manager_process_barrier_fd() function.
benjarobin
force-pushed
the
perf_barrier_fd
branch
from
7bff205
to
5151b4c
Compare
|
@poettering Force pushed, add a check on |
This function is called for each message, and almost all of them will not
contain BARRIER=1. So 5 comparaisons are realized in previous code. Now the buffer is
parsed only once.
If BARRIER=1 is found, drop the message in all case. So always return true
when BARRIER=1 is found at the beginning of a new line
Follow up of #15547