Page MenuHomeFreeBSD

Add setproctitle_fast() for frequent callers.
ClosedPublic

Authored by munro_ip9.org on Jul 3 2018, 12:56 PM.

Details

Summary

"Some applications, notably PostgreSQL, want to call setproctitle()
very often. It's slow. Provide an alternative cheap way of updating
process titles without making any syscalls, instead requiring other
processes (top, ps etc) to do a bit more work to retrieve the data.
This uses a pre-existing code path inherited from ancient BSD, which
always did it that way."

As discussed in https://lists.freebsd.org/pipermail/freebsd-hackers/2018-July/052973.html .

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

munro_ip9.org created this revision.Jul 3 2018, 12:56 PM
kib added a comment.Jul 3 2018, 2:02 PM

You need to add the new symbol to the gen/Symbol.map, otherwise it is not possible to link to it.

Please use -U99999 argument to the diff. eg. svn diff -x -U999999 when uploading the diff, to provide full context.

lib/libc/gen/setproctitle.3
45 ↗(On Diff #44801)

... slower, by not updating the kernel cache of the program arguments.

lib/libc/gen/setproctitle.c
58 ↗(On Diff #44801)

I do not think that inline is needed there, compilers are very eager to inline on their own.

76 ↗(On Diff #44801)

return (NULL);

83 ↗(On Diff #44801)

Same. I do not repeat this note any more.

177 ↗(On Diff #44801)

if (buf && !fast_update) {

mjg added a subscriber: mjg.Jul 3 2018, 3:16 PM
mjg added inline comments.
lib/libc/gen/setproctitle.c
183 ↗(On Diff #44801)

It's not a requirement for this patch, but it would be nice to sort out kernel-side while doing work in the area. In particular, there is no reason to grab the pid -- passing 0 should be treated as "this process".

Updates from kib's code review.

munro_ip9.org marked 5 inline comments as done.Jul 3 2018, 11:51 PM
munro_ip9.org added inline comments.
lib/libc/gen/setproctitle.c
58 ↗(On Diff #44801)

clang 6.0 -O2 doesn't seem to inline it whether I say that or not. I could use attribute((always_inline)) but that seems unorthodox. I just removed it.

munro_ip9.org marked an inline comment as done.Jul 3 2018, 11:51 PM
munro_ip9.org marked an inline comment as done.Jul 4 2018, 2:57 AM
munro_ip9.org added inline comments.
lib/libc/gen/setproctitle.c
183 ↗(On Diff #44801)

Good point. I will look into writing a separate patch for that.

kib accepted this revision.Jul 4 2018, 12:52 PM

I will fix style issues myself, but I think it is useful for you to see them listed explicitly.
I am going to commit this now.

lib/libc/gen/setproctitle.3
41 ↗(On Diff #44834)

New sentence requires new line.

lib/libc/gen/setproctitle.c
158 ↗(On Diff #44834)

return ();

This revision is now accepted and ready to land.Jul 4 2018, 12:52 PM
This revision was automatically updated to reflect the committed changes.
kib reopened this revision.Jul 4 2018, 1:23 PM
This revision was not accepted when it landed; it landed in state Needs Review.Jul 4 2018, 1:28 PM
This revision was automatically updated to reflect the committed changes.
seanc added a subscriber: seanc.Jul 4 2018, 3:39 PM
munro_ip9.org marked an inline comment as done.Jul 4 2018, 10:33 PM

Thanks kib! For the record, here's a patch for PostgreSQL that I plan to push when this makes it to a RELEASE:

https://www.postgresql.org/message-id/CAEepm%3D1wKMTi81uodJ%3D1KbJAz5WedOg%3Dcr8ewEXrUFeaxWEgww%40mail.gmail.com

kib reopened this revision.Jul 5 2018, 8:54 AM

Thanks kib! For the record, here's a patch for PostgreSQL that I plan to push when this makes it to a RELEASE:

https://www.postgresql.org/message-id/CAEepm%3D1wKMTi81uodJ%3D1KbJAz5WedOg%3Dcr8ewEXrUFeaxWEgww%40mail.gmail.com

Why waiting for a release ? There is a significant population of users utilizing stable, and even HEAD is sometimes used not only by developers. Early you push this into the wild world, more feedback you get.

In D16111#342204, @kib wrote:

Thanks kib! For the record, here's a patch for PostgreSQL that I plan to push when this makes it to a RELEASE:

https://www.postgresql.org/message-id/CAEepm%3D1wKMTi81uodJ%3D1KbJAz5WedOg%3Dcr8ewEXrUFeaxWEgww%40mail.gmail.com

Why waiting for a release ? There is a significant population of users utilizing stable, and even HEAD is sometimes used not only by developers. Early you push this into the wild world, more feedback you get.

https://reviews.freebsd.org/D16234 to add the patches to the FreeBSD ports for stable PostgreSQL releases. I'll commit the change to the PostgreSQL development branch soon, to affect PostgreSQL 12.

I'll commit the change to the PostgreSQL development branch soon, to affect PostgreSQL 12.

Done.

munro_ip9.org accepted this revision.Aug 30 2018, 10:12 PM
This revision is now accepted and ready to land.Aug 30 2018, 10:12 PM