Page MenuHome

Thread naming support (for debugging)
AcceptedPublic

Authored by Nick Renieris (velocity) on Dec 11 2018, 3:55 AM.

Details

Summary

Description

This patch adds:

  • A pool_name argument to BLI_threadpool_init, which is used to name the thread that gets created
  • A utility function called BLI_thread_name_set for setting the name of the currently running thread from anywhere

The latter is used in blenlib's Task Scheduler, additionally denoting whether the Task Scheduler in question is currently waiting or running.

The former is of course an API breaking change, so the patch touches a handful of projects.

Compatibility

The code is written for Windows (MSVC), Linux, FreeBSD and macOS and tested on Windows (thread names appear in the Thread list).

Rationale

The is a debugging aid, which is why it's only enabled on Debug builds (and for Windows, specifically when the MSVC debugger is attached).

Diff Detail

Repository
rB Blender

Event Timeline

Sergey Sharybin (sergey) requested changes to this revision.Dec 11 2018, 9:21 AM

Generally fine, but see comment in the task.c.
Would really prefer to not have any overhead there, even for debug-only builds.

source/blender/blenlib/BLI_threads.h
85

As per style guide, const char *thread_name

source/blender/blenlib/BLI_winstuff.h
134 ↗(On Diff #12846)

I am not sure we should have this:

  • Is not following our naming convention
  • Is used in one single place
source/blender/blenlib/intern/task.c
441 ↗(On Diff #12846)

I'm not sure this is a great idea to change thread name in a very hot function. Surely, is not such of a problem for release builds, but we do debug complex production scenes in a debug mode.

Also, this doesn't bring a lot of new information and is not complete information. Thread which does wait_and_work is not renamed and there you can not reliably have this wait/run semantic working without having some sort of TLS stack of all names. Don't even think it worth it.

source/blender/blenlib/intern/threads.c
828
  • Put { at the same line
  • Would prefer to have early output rather than having a whole block indented.
830

NDEBUG is already checked.

Sybren A. Stüvel (sybren) added inline comments.
source/blender/blenlib/intern/threads.c
212

Is pool_name this the name of the pool or the name of the thread? It seems to be used as both, which I find confusing.

Brecht Van Lommel (brecht) added inline comments.
source/blender/blenlib/intern/threads.c
215

Don't use unnecessary abbreviations like "TPool" or "Sched".

source/blender/compositor/intern/COM_WorkScheduler.cpp
134

Rename to "Compositor CPU" or something like that, this is not a general work scheduler.

Nick Renieris (velocity) marked 12 inline comments as done.Dec 11 2018, 3:36 PM
Nick Renieris (velocity) added inline comments.
source/blender/blenlib/BLI_threads.h
85

Typo, thank you.

source/blender/blenlib/BLI_winstuff.h
134 ↗(On Diff #12846)

It's copied verbatim from here. Guess I should still change the style.

It's a function only to keep the ugliness confined to BLI_winstuff.cpp.

Better than more #ifdefing in threads.c, no?

source/blender/blenlib/intern/task.c
441 ↗(On Diff #12846)

Yeah that makes sense, I'll remove it.

source/blender/blenlib/intern/threads.c
212

Well it's the pool name, but it's only use by the implementation is to set it as a thread name.

You're right, I'll just name it thread_name, since I also need to make the < 15 chars requirement clear.

215

Yeah I'd like to use more expressive names, but it's a POSIX limitation that the thread name can't be longer than 15 characters, so I had to abbreviate.

I should probably mention this in the documentation for BLI_threadpool_init too (after I rename it to thread_name).

Alternatively, I could not have this limitatin for the threadpool API and cull to the last 15 characters (or something like that) when naming the thread, what do you think?

830

I fixed this yesterday, I probably messed something up updating the patch.

Nick Renieris (velocity) marked 4 inline comments as done.

Tended to resolved review comments, awaiting more feedback.

Not sure how much information this really adds, It's pretty clear the code is running in a threadpool by simply looking at the callstack. And the first question you then generally have is 'who queued this?' which this doesn't answer, (we could store a callstack i suppose from the location that pushed the work, but i doubt it's worth the overhead) however if it doesn't break anything, and people find it useful, i have nothing against naming the threads.

I'm not a super huge fan of the windows specific codepaths, i took a quick gander and there seems to be a newer version of pthraeds4w that supports pthread_setname_np on windows , given this is a windows only dep i'm ok updating it, but it's up to @Brecht Van Lommel (brecht) and @Sergey Sharybin (sergey) to decide if it's too late in 2.8 to still change such a core lib at this point in time.

It's fine to update the pthread library still, we're not that close to a release.

Ok, do I have to do this and if so, what's the process of updating svn deps?

And yeah @LazyDodo (LazyDodo) there would be some neat things that could be done, if it wasn't for the annoying 15 char POSIX limitation.

No need to worry about that, I'll do it, and will let you know here when it's done.

And yeah @LazyDodo (LazyDodo) there would be some neat things that could be done, if it wasn't for the annoying 15 char POSIX limitation.

I was honestly not suggesting putting a stacktrace in the thread name :)

And yeah @LazyDodo (LazyDodo) there would be some neat things that could be done, if it wasn't for the annoying 15 char POSIX limitation.

I was honestly not suggesting putting a stacktrace in the thread name :)

Not a stacktrace :D but maybe something more lightweight and definitely more descriptive thread names at least.

Nick Renieris (velocity) marked an inline comment as done.Dec 11 2018, 9:25 PM
Nick Renieris (velocity) added inline comments.
source/blender/blenlib/intern/task.c
432 ↗(On Diff #12861)

this is fixed locally

Nick Renieris (velocity) marked an inline comment as done.Dec 12 2018, 12:43 AM

Should be good to go now.

Nick Renieris (velocity) marked an inline comment as done.Dec 12 2018, 2:31 PM

It's been more than a month, can someone re-review or merge this?

2+ months, still no review.
Is anybody interested in this at all or should I just close it...?

I updated pthreads for this so I prefer if it didn't get abandoned, but @Brecht Van Lommel (brecht) or @Sergey Sharybin (sergey) have the final call here.

This revision is now accepted and ready to land.Tue, Feb 26, 10:32 PM

Would need to be applied in 2.7 and then merged to master.
Other than minor thing seems fine to me.

source/blender/blenlib/intern/threads.c
821

Seems that thread_name will be considered unused with pedantic compiler? Easy to fix when applying though.