Make set_ps_display faster and easier to use
While doing some benchmarking of some fast-to-execute queries, I see
that set_ps_display() popping up on the profiles. Looking a little
deeper, there are some inefficiencies in there that we could fix.
For example, the following is pretty poor:
strlcpy(ps_buffer + ps_buffer_fixed_size, activity,
ps_buffer_size - ps_buffer_fixed_size);
ps_buffer_cur_len = strlen(ps_buffer);
We already know the strlen of the fixed-sized part, so why bother
doing strlen on the entire thing? Also, if we did just do
strlen(activity), we could just memcpy, which would be much faster
than strlcpy's byte-at-a-time method of copying.
Adjusting that lead me to notice that we often just pass string
constants to set_ps_display(), so we already know the strlen for this
at compile time. So maybe we can just have set_ps_display_with_len()
and then make a static inline wrapper that does strlen() so that when
the compiler can figure out the length, it just hard codes it.
After doing that, I went over all usages of set_ps_display() to see if
any of those call sites knew the length already in a way that the
compiler wouldn't be able to deduce. There were a few cases to adjust
when setting the process title to contain the command tag.
After fixing up the set_ps_display()s to use set_ps_display_with_len()
where possible, I discovered some not so nice code which appends "
waiting" onto the process title. Basically, there's a bunch of code
that looks like this:
const char *old_status;
int len;
old_status = get_ps_display(&len);
new_status = (char *) palloc(len + 8 + 1);
memcpy(new_status, old_status, len);
strcpy(new_status + len, " waiting");
set_ps_display(new_status);
new_status[len] = '\0'; /* truncate off " waiting" */
Seeing that made me wonder if we shouldn't just have something more
generic for setting a suffix on the process title. I came up with
set_ps_display_suffix() and set_ps_display_remove_suffix(). The above
code can just become:
set_ps_display_suffix("waiting");
then to remove the "waiting" suffix, just:
set_ps_display_remove_suffix();
I considered adding a format version to append the suffix as there's
one case that could make use of it, but in the end, decided it might
be overkill, so I left that code like:
char buffer[32];
sprintf(buffer, "waiting for %X/%X", LSN_FORMAT_ARGS(lsn));
set_ps_display_suffix(buffer);
I don't think that's terrible enough to warrant making a va_args
version of set_ps_display_suffix(), especially for just 1 instance of
it.
I also resisted making set_ps_display_suffix_with_len(). The new code
should be quite a bit
faster already without troubling over that additional function.
I've attached the patch.
David
Attachments:
optimize_and_make_set_ps_display_better.patchtext/plain; charset=US-ASCII; name=optimize_and_make_set_ps_display_better.patchDownload+191-86
Hi,
On 2023-02-16 14:19:24 +1300, David Rowley wrote:
After fixing up the set_ps_display()s to use set_ps_display_with_len()
where possible, I discovered some not so nice code which appends "
waiting" onto the process title. Basically, there's a bunch of code
that looks like this:const char *old_status;
int len;old_status = get_ps_display(&len);
new_status = (char *) palloc(len + 8 + 1);
memcpy(new_status, old_status, len);
strcpy(new_status + len, " waiting");
set_ps_display(new_status);
new_status[len] = '\0'; /* truncate off " waiting" */
Yea, that code is atrocious... It took me a while to figure out that no,
LockBufferForCleanup() isn't leaking memory, because it'll always reach the
cleanup path *further up* in the function.
Avoiding the allocation across loop iterations seems like a completely
pointless optimization in these paths - we add the " waiting", precisely
because it's a slow path. But of course not allocating memory would be even
better...
Seeing that made me wonder if we shouldn't just have something more
generic for setting a suffix on the process title. I came up with
set_ps_display_suffix() and set_ps_display_remove_suffix(). The above
code can just become:set_ps_display_suffix("waiting");
then to remove the "waiting" suffix, just:
set_ps_display_remove_suffix();
That'd definitely be better.
It's not really a topic for this patch, but somehow the fact that we have
these set_ps_display() calls all over feels wrong, particularly because most
of them are paired with a pgstat_report_activity() call. It's not entirely
obvious how it should be instead, but it doesn't feel right.
+/* + * set_ps_display_suffix + * Adjust the process title to append 'suffix' onto the end with a space + * between it and the current process title. + */ +void +set_ps_display_suffix(const char *suffix) +{ + size_t len;
Think this will give you an unused-variable warning in the PS_USE_NONE case.
+#ifndef PS_USE_NONE + /* update_process_title=off disables updates */ + if (!update_process_title) + return; + + /* no ps display for stand-alone backend */ + if (!IsUnderPostmaster) + return; + +#ifdef PS_USE_CLOBBER_ARGV + /* If ps_buffer is a pointer, it might still be null */ + if (!ps_buffer) + return; +#endif
This bit is now repeated three times. How about putting it into a helper?
+#ifndef PS_USE_NONE +static void +set_ps_display_internal(void)
Very very minor nit: Perhaps this should be update_ps_display() or
flush_ps_display() instead?
Greetings,
Andres Freund
Thank you for having a look at this.
On Fri, 17 Feb 2023 at 14:01, Andres Freund <andres@anarazel.de> wrote:
+set_ps_display_suffix(const char *suffix) +{ + size_t len;Think this will give you an unused-variable warning in the PS_USE_NONE case.
Fixed
+#ifndef PS_USE_NONE + /* update_process_title=off disables updates */ + if (!update_process_title) + return; + + /* no ps display for stand-alone backend */ + if (!IsUnderPostmaster) + return; + +#ifdef PS_USE_CLOBBER_ARGV + /* If ps_buffer is a pointer, it might still be null */ + if (!ps_buffer) + return; +#endifThis bit is now repeated three times. How about putting it into a helper?
Good idea. Done.
+set_ps_display_internal(void)
Very very minor nit: Perhaps this should be update_ps_display() or
flush_ps_display() instead?
I called the precheck helper update_ps_display_precheck(), so went
with flush_ps_display() for updating the display so they both didn't
start with "update".
Updated patch attached.
David