Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add Thread.setName and Thread.getName #8570

Merged
merged 4 commits into from Jul 29, 2021
Merged

Conversation

vrischmann
Copy link
Contributor

This PR adds Thread.setName and Thread.getName.

It's not complete, I only tested this on linux. I spent a bit of time researching what other platforms do and from what I can tell setting/getting a thread's name is supported on linux, windows (since Windows 10), macos, openbsd, freebsd, netbsd.
Annoyingly since it's non standard there are some slight differences between implementations of pthread_setname_np.

Before implementing everything I'd like to get some early feedback on the API.

@LemonBoy
Copy link
Contributor

API-wise I think it makes sense to add an extra struct parameter to the thread creation function:

struct CreateThreadOptions = struct {
    thread_name: ?[:0]const u8 = null,
}

There's little to no point to setting the name after the thread is created.

lib/std/Thread.zig Outdated Show resolved Hide resolved
lib/std/Thread.zig Outdated Show resolved Hide resolved
lib/std/c.zig Outdated Show resolved Hide resolved
@daurnimator
Copy link
Collaborator

There's little to no point to setting the name after the thread is created.

It can be useful to name worker threads after what they are currently doing.

This also has implications in schedulers where workloads are not locked to specific threads.

@vrischmann vrischmann force-pushed the thread-name branch 4 times, most recently from 4ef68ab to 24b03c2 Compare April 19, 2021 23:18
@vrischmann
Copy link
Contributor Author

I've implemented linux with or without pthreads, macOS and netbsd/freebsd/openbsd. I've only been able to test linux at the moment, I'll be able to test freebsd and probably netbsd.

I'm still working on the windows implementation.

I'll make the API change last.

@vrischmann vrischmann marked this pull request as draft April 19, 2021 23:52
Copy link
Contributor

@semarie semarie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The return type for functions for OpenBSD should be void. It will make it different from .freebsd for setting the name, and different from others for getting it.

lib/std/c/openbsd.zig Outdated Show resolved Hide resolved
lib/std/Thread.zig Outdated Show resolved Hide resolved
lib/std/Thread.zig Outdated Show resolved Hide resolved
lib/std/Thread.zig Show resolved Hide resolved
lib/std/Thread.zig Outdated Show resolved Hide resolved
lib/std/Thread.zig Outdated Show resolved Hide resolved
lib/std/Thread.zig Outdated Show resolved Hide resolved
lib/std/Thread.zig Outdated Show resolved Hide resolved
lib/std/Thread.zig Outdated Show resolved Hide resolved
lib/std/Thread.zig Outdated Show resolved Hide resolved
lib/std/Thread.zig Outdated Show resolved Hide resolved
lib/std/Thread.zig Outdated Show resolved Hide resolved
lib/std/Thread.zig Outdated Show resolved Hide resolved
@vrischmann
Copy link
Contributor Author

I've addressed most of the comments in the latest commit (minus those still in discussion).

I successfully tested this on FreeBSD, however I can't test on NetBSD or OpenBSD because of compilation errors somewhere else in the stdlib.

@vrischmann
Copy link
Contributor Author

I've added the windows implementation.

One thing I'm not sure about is the utf8 -> utf16 conversion, right now I'm doing this:

           var name_buf_w: [maxNameLen:0]u16 = undefined;
           const length = try std.unicode.utf8ToUtf16Le(&name_buf_w, name);

The utf8 name can't be bigger than maxNameLen so I was assuming a [maxNameLen:0]u16 is always enough to fit the utf16 data but I'm not sure if that's correct, I'm not super familiar with UTF16.

lib/std/Thread.zig Outdated Show resolved Hide resolved
lib/std/Thread.zig Outdated Show resolved Hide resolved
lib/std/Thread.zig Outdated Show resolved Hide resolved
lib/std/Thread.zig Outdated Show resolved Hide resolved
lib/std/Thread.zig Outdated Show resolved Hide resolved
@vrischmann vrischmann force-pushed the thread-name branch 4 times, most recently from 8c9c7ee to 1fedd74 Compare April 26, 2021 16:27
@vrischmann vrischmann marked this pull request as ready for review April 26, 2021 16:32
@vrischmann vrischmann force-pushed the thread-name branch 2 times, most recently from 22cd76d to d91180b Compare April 26, 2021 18:59
@PhaseMage
Copy link
Contributor

One thing I'm not sure about is the utf8 -> utf16 conversion, right now I'm doing this:

           var name_buf_w: [maxNameLen:0]u16 = undefined;
           const length = try std.unicode.utf8ToUtf16Le(&name_buf_w, name);

The utf8 name can't be bigger than maxNameLen so I was assuming a [maxNameLen:0]u16 is always enough to fit the utf16 data but I'm not sure if that's correct, I'm not super familiar with UTF16.

This is correct. A string of 'n' UTF16 code units can always hold 'n' UTF8 code units.

The normal issue is only the other way around, where one UTF16 code unit might require multiple UTF8 code units of storage. For ASCII code points, this isn't relevant. But for larger code points (eg. Emoji), this can be important. I suspect that Windows supports a maximum number of UTF16 code units. Since you allocate to implement getName(), this works fine. However, limiting maxNameLen to 31 UTF8 code units artificially limits the number of code points you can give to setName() (say, a string of Emoji). But using Emoji in thread names is probably a bad idea anyway since debuggers might not support it well :-)

@vrischmann
Copy link
Contributor Author

One thing I'm not sure about is the utf8 -> utf16 conversion, right now I'm doing this:

           var name_buf_w: [maxNameLen:0]u16 = undefined;
           const length = try std.unicode.utf8ToUtf16Le(&name_buf_w, name);

The utf8 name can't be bigger than maxNameLen so I was assuming a [maxNameLen:0]u16 is always enough to fit the utf16 data but I'm not sure if that's correct, I'm not super familiar with UTF16.

This is correct. A string of 'n' UTF16 code units can always hold 'n' UTF8 code units.

The normal issue is only the other way around, where one UTF16 code unit might require multiple UTF8 code units of storage. For ASCII code points, this isn't relevant. But for larger code points (eg. Emoji), this can be important. I suspect that Windows supports a maximum number of UTF16 code units. Since you allocate to implement getName(), this works fine. However, limiting maxNameLen to 31 UTF8 code units artificially limits the number of code points you can give to setName() (say, a string of Emoji). But using Emoji in thread names is probably a bad idea anyway since debuggers might not support it well :-)

Thanks. I actually tested emojis in the thread name, Visual Studio 2019 supports them at least.
I agree that a maxNameLen of 31 is arbitrary but in my opinion it should be enough, and it's in line with what other platforms do.
I could try and see if the call to SetThreadDescription fails with a bigger maxNameLen and get a better idea where the limit is, because I couldn't find it in the documentation.

@vrischmann vrischmann force-pushed the thread-name branch 2 times, most recently from 437c367 to 70afb6f Compare May 30, 2021 20:39
@vrischmann vrischmann force-pushed the thread-name branch 2 times, most recently from 6c2ff78 to 7a57207 Compare June 9, 2021 20:25
@Vexu Vexu added the standard library This issue involves writing Zig code for the standard library. label Jun 13, 2021
Copy link
Member

@andrewrk andrewrk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your patience; I know it took a long time to get a review from me.

I have a couple requests and then I think this looks good to merge.

Can you please rebase against master branch and fix the conflicts in addition to the requested changes?

lib/std/Thread.zig Outdated Show resolved Hide resolved
lib/std/Thread.zig Outdated Show resolved Hide resolved
Copy link
Member

@andrewrk andrewrk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the fixups! Found 2 more things - sorry for not finding them last time, but at least now I am paying close attention, and will give this a prompt merge :)

lib/std/Thread.zig Outdated Show resolved Hide resolved
lib/std/Thread.zig Outdated Show resolved Hide resolved
@vrischmann
Copy link
Contributor Author

Ok, addressed your last feedback.

I had to change some logic for musl since the type returned by getCurrentId changed but it's working now.
I've only been able to test linux/windows, I can test freebsd later.
I don't have a mac to test on macOS unfortunately.

@vrischmann
Copy link
Contributor Author

Pushed the last fixes for macos, freebsd/netbsd.

@andrewrk andrewrk merged commit 192b5d2 into ziglang:master Jul 29, 2021
@vrischmann vrischmann deleted the thread-name branch July 29, 2021 23:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
standard library This issue involves writing Zig code for the standard library.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants