Bug 166678 – Use ThreadName to specify long and short names

• Component: Web Template Framework
• Status: NEW

[Yusuke Suzuki]

...

[Yusuke Suzuki]

Created attachment 298009 [details]
Patch

[Michael Catanzaro]

Comment on attachment 298009 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=298009&action=review

> Source/WTF/wtf/AutomaticThread.h:134
> +    AutomaticThread(const LockHolder&, Box<Lock>, RefPtr<AutomaticThreadCondition>, ASCIILiteral threadName = ASCIILiteral("WTF::AutomaticThread"));

It seems desirable to force users to name the threads, so I would omit the default value here. Your call.

> Source/WTF/wtf/ParallelHelperPool.h:183
> +    WTF_EXPORT_PRIVATE ParallelHelperPool(ASCIILiteral threadName = ASCIILiteral("WTF::ParallelHelperPool"));

Ditto.

[Darin Adler]

Please keep in mind that thread names will be seen by many, many people who are not working on WebKit and they will need context so they can understand these are part of WebKit or JavaScriptCore or whatever. Given that, I think we need to carefully choose thread names that are easy to understand. I would like to review them as a set and make sure they are clear.

[Michael Catanzaro]

It should be pretty clear that the threads are WebKit threads when the base frames are all inside WebKit, no...?

I understand the desire for more-descriptive names on Darwin, which is difficult to reconcile with the very low character limit on Linux. One option is to require passing a long name and a short name for each thread, that way the long name can be used on Darwin and the shorter names on Linux and Windows.

[Darin Adler]

(In reply to comment #4)
> It should be pretty clear that the threads are WebKit threads when the base
> frames are all inside WebKit, no...?

The point here is to quickly help someone understand what’s going on. Quickly give you the right idea.

It’s true that if you are familiar with the symbol names from WebKit and its related frameworks, have symbols in your backtrace, and can quickly look over the backtrace ignoring the unimportant frames and spotting the significant ones, then you can determine it’s a WebKit-created thread. But that’s actually the problem statement!

[Yusuke Suzuki]

(In reply to comment #5)
> (In reply to comment #4)
> > It should be pretty clear that the threads are WebKit threads when the base
> > frames are all inside WebKit, no...?
> 
> The point here is to quickly help someone understand what’s going on.
> Quickly give you the right idea.
> 
> It’s true that if you are familiar with the symbol names from WebKit and its
> related frameworks, have symbols in your backtrace, and can quickly look
> over the backtrace ignoring the unimportant frames and spotting the
> significant ones, then you can determine it’s a WebKit-created thread. But
> that’s actually the problem statement!

I think the names picked for this patch is OK. "jsc.dfg-worklist" is descriptive I think.
And after https://bugs.webkit.org/show_bug.cgi?id=166676 patch is landed, in Linux environment, this name will become "dfg-worklist" due to 15 characters limitation. But we still keep "jsc.dfg-worklist" name in macOS!

BTW, I found so many different thread name forms in the current WebKit.
I think it is worth discussing in the mailing list.
Once this, and the above thread name patch is landed, I'll post the mail about the thread name conventions in WebKit.

[Darin Adler]

(In reply to comment #6)
> "jsc.dfg-worklist" is descriptive I think.

We are definitely thinking differently about these names! That name would mean nothing to me as a programmer who just happened to be using WebKit. Neither "jsc" nor "dfg" sound like they have anything to do with WebKit.

[Yusuke Suzuki]

(In reply to comment #7)
> (In reply to comment #6)
> > "jsc.dfg-worklist" is descriptive I think.
> 
> We are definitely thinking differently about these names! That name would
> mean nothing to me as a programmer who just happened to be using WebKit.
> Neither "jsc" nor "dfg" sound like they have anything to do with WebKit.

I see. So we should have some good name policy for that.
There are several names in the current ToT WebKit. I picked some of them.

In WebCore,
    IDB server is "IndexedDatabase Server"
    AsyncAudioDecoder is "Audio Decoder"
    GCController is "WebCore: GCController"
    Icon Database is "WebCore: IconDatabase"
    Audio's ReverbConvolver is "convolution background thread"
The thread name used by WorkQueue in WebCore is,
    Crypto Queue is "com.apple.WebKit.CryptoQueue"
    Image decoder is "org.webkit.ImageDecoder"
    Blob utility is "org.webkit.BlobUtility"
    Data URL decoder is "org.webkit.DataURLDecoder"
In JSC
    Before this patch, all the AutomaticThreads (including JIT worklist / DFG worklist) is "WTF::AutomaticThread"
    Super Sampler thread is "JSC Super Sampler"
    Asychronous Disasm is "Asynchronous Disassembler"
    Sampling profiler is "jsc.sampling-profiler.thread"
    WASM compiler thread is "jsc.wasm-b3-compilation.thread"
In WebKit2
    Network Cache is "IOChannel::readSync"
    IPC workqueue is "com.apple.IPC.ReceiveQueue"

We should have consistent name policy for that. Some of the currently used threads do not include any information related to WebKit.

I think the most descriptive thing is WorkQueue's name like "org.webkit.ImageDecoder". It includes the information, like

1. It is related to WebKit
2. It is related to ImageDecoder

And these forms are correctly handled in Linux. In Linux, the above name will be converted to "ImageDecoder".

So, I suggest the name form, like,

"org.webkit.jsc.BaselineCompiler"
"org.webkit.jsc.DFGCompiler"
"org.webkit.jsc.HeapHelper" (This can be used for any heap related purpose)

[Yusuke Suzuki]

(In reply to comment #8)
> (In reply to comment #7)
> > (In reply to comment #6)
> > > "jsc.dfg-worklist" is descriptive I think.
> > 
> > We are definitely thinking differently about these names! That name would
> > mean nothing to me as a programmer who just happened to be using WebKit.
> > Neither "jsc" nor "dfg" sound like they have anything to do with WebKit.
> 
> I see. So we should have some good name policy for that.
> There are several names in the current ToT WebKit. I picked some of them.
> 
> In WebCore,
>     IDB server is "IndexedDatabase Server"
>     AsyncAudioDecoder is "Audio Decoder"
>     GCController is "WebCore: GCController"
>     Icon Database is "WebCore: IconDatabase"
>     Audio's ReverbConvolver is "convolution background thread"
> The thread name used by WorkQueue in WebCore is,
>     Crypto Queue is "com.apple.WebKit.CryptoQueue"
>     Image decoder is "org.webkit.ImageDecoder"
>     Blob utility is "org.webkit.BlobUtility"
>     Data URL decoder is "org.webkit.DataURLDecoder"
> In JSC
>     Before this patch, all the AutomaticThreads (including JIT worklist /
> DFG worklist) is "WTF::AutomaticThread"
>     Super Sampler thread is "JSC Super Sampler"
>     Asychronous Disasm is "Asynchronous Disassembler"
>     Sampling profiler is "jsc.sampling-profiler.thread"
>     WASM compiler thread is "jsc.wasm-b3-compilation.thread"
> In WebKit2
>     Network Cache is "IOChannel::readSync"
>     IPC workqueue is "com.apple.IPC.ReceiveQueue"
> 
> We should have consistent name policy for that. Some of the currently used
> threads do not include any information related to WebKit.
> 
> I think the most descriptive thing is WorkQueue's name like
> "org.webkit.ImageDecoder". It includes the information, like
> 
> 1. It is related to WebKit
> 2. It is related to ImageDecoder
> 
> And these forms are correctly handled in Linux. In Linux, the above name
> will be converted to "ImageDecoder".
> 
> So, I suggest the name form, like,
> 
> "org.webkit.jsc.BaselineCompiler"
> "org.webkit.jsc.DFGCompiler"
> "org.webkit.jsc.HeapHelper" (This can be used for any heap related purpose)

Unfortunately, BaselineCompiler is 16 length. "BaselineCompile" is also OK I think.

[Yusuke Suzuki]

Or "JITCompiler"

[Yusuke Suzuki]

Created attachment 298059 [details]
Patch

[Yusuke Suzuki]

I've posted the mail about thread naming policy.
https://lists.webkit.org/pipermail/webkit-dev/2017-January/028585.html

[Yusuke Suzuki]

I'll rename this "jsc" to "JavaScriptCore".
This is based on the directory name of the module.

[Yusuke Suzuki]

Created attachment 298072 [details]
Patch

[Yusuke Suzuki]

Created attachment 298191 [details]
Patch

[Yusuke Suzuki]

Created attachment 298193 [details]
Patch

[Yusuke Suzuki]

Created attachment 298350 [details]
Patch

[Yusuke Suzuki]

Created attachment 298354 [details]
Patch

[Konstantin Tokarev]

I think this could be done without macro. For example, WTF::createThread can be replaced with inline wrapper, taking two const char* arguments, and choosing one of them with #if. Same for other functions taking thread names as C string, there seems to be not many of them (I've spotted initializeCurrentThreadInternal and WorkQueue::create)

[Yusuke Suzuki]

(In reply to comment #19)
> I think this could be done without macro. For example, WTF::createThread can
> be replaced with inline wrapper, taking two const char* arguments, and
> choosing one of them with #if. Same for other functions taking thread names
> as C string, there seems to be not many of them (I've spotted
> initializeCurrentThreadInternal and WorkQueue::create)

The reason why I use the macro here is that we would like to add the prefix `"WebKit: "` without allocating additional memory.

[Konstantin Tokarev]

Got it, though this allocation overhead can probably be neglected as compared to thread creation that follows.

[Brady Eidson]

(In reply to comment #20)
> (In reply to comment #19)
> > I think this could be done without macro. For example, WTF::createThread can
> > be replaced with inline wrapper, taking two const char* arguments, and
> > choosing one of them with #if. Same for other functions taking thread names
> > as C string, there seems to be not many of them (I've spotted
> > initializeCurrentThreadInternal and WorkQueue::create)
> 
> The reason why I use the macro here is that we would like to add the prefix
> `"WebKit: "` without allocating additional memory.

Why do we think this is important?

I suspect it's not (e.g. The computational and memory overhead of creating a thread will always be *substantially* more expensive than a simple string concat)

And I truly hate the way a new ALL_CAPS_MACRO looks in code in comparison to just creating an object.

[Yusuke Suzuki]

(In reply to comment #22)
> (In reply to comment #20)
> > (In reply to comment #19)
> > > I think this could be done without macro. For example, WTF::createThread can
> > > be replaced with inline wrapper, taking two const char* arguments, and
> > > choosing one of them with #if. Same for other functions taking thread names
> > > as C string, there seems to be not many of them (I've spotted
> > > initializeCurrentThreadInternal and WorkQueue::create)
> > 
> > The reason why I use the macro here is that we would like to add the prefix
> > `"WebKit: "` without allocating additional memory.
> 
> Why do we think this is important?
> 
> I suspect it's not (e.g. The computational and memory overhead of creating a
> thread will always be *substantially* more expensive than a simple string
> concat)
> 
> And I truly hate the way a new ALL_CAPS_MACRO looks in code in comparison to
> just creating an object.

The largest reason I used this macro is that this does not allocate any memory even if we use it in bmalloc's AsyncTask.
I don't like allocating memory dynamically in the malloc implementation.

[Yusuke Suzuki]

(In reply to comment #23)
> (In reply to comment #22)
> > (In reply to comment #20)
> > > (In reply to comment #19)
> > > > I think this could be done without macro. For example, WTF::createThread can
> > > > be replaced with inline wrapper, taking two const char* arguments, and
> > > > choosing one of them with #if. Same for other functions taking thread names
> > > > as C string, there seems to be not many of them (I've spotted
> > > > initializeCurrentThreadInternal and WorkQueue::create)
> > > 
> > > The reason why I use the macro here is that we would like to add the prefix
> > > `"WebKit: "` without allocating additional memory.
> > 
> > Why do we think this is important?
> > 
> > I suspect it's not (e.g. The computational and memory overhead of creating a
> > thread will always be *substantially* more expensive than a simple string
> > concat)
> > 
> > And I truly hate the way a new ALL_CAPS_MACRO looks in code in comparison to
> > just creating an object.
> 
> The largest reason I used this macro is that this does not allocate any
> memory even if we use it in bmalloc's AsyncTask.
> I don't like allocating memory dynamically in the malloc implementation.

Or, another way is just using such a macro for bmalloc and use `ThreadName("longName", "shortName")` elsewhere.
I think it's ok.

[Darin Adler]

Comment on attachment 298354 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=298354&action=review

I would like to review the set of revised thread names separately from the patch that introduces all of them. Just a list. Maybe even on webkit-dev.

I think all the threads that are for JavaScriptCore need to have one of "JavaScript", "JavaScriptCore", or "JS" in their names for clarity. Maybe even "JavaScriptCore:" instead of "WebKit:", but maybe "WebKit: JS xxx" would be enough context.

I think we should bias a lot more toward an approach that lets us write thread names that are a single string except in the rare cases where we can’t do that. As discussed elsewhere, we should be willing to do a little runtime work to prepend the "WebKit: " prefix to the long time, to strip spaces to make the short name. We might event do something exotic like choosing a meta-character to say "start/end of short name" if a lot of short names are substrings (minus spaces) of the long string.

Then we would only provide a separate short string in the rare case where none of those tricks will work. One downside to my ideas is that we would not know at compile time if the short name is short enough, but otherwise, I think those ideas will work well.

> Source/WTF/wtf/text/ASCIILiteral.h:30
> +class ASCIILiteral {

Great to move this into its own separate file.

Some day I would like to figure out how to make it so actual compiler literals can turn into this automatically, and we only need to specify it explicitly when the variable goes through some layer that just calls it a const char*. No idea if there is any language mechanism that could make that practical some day.

[Yusuke Suzuki]

Comment on attachment 298354 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=298354&action=review

>> Source/WTF/wtf/text/ASCIILiteral.h:30
>> +class ASCIILiteral {
> 
> Great to move this into its own separate file.
> 
> Some day I would like to figure out how to make it so actual compiler literals can turn into this automatically, and we only need to specify it explicitly when the variable goes through some layer that just calls it a const char*. No idea if there is any language mechanism that could make that practical some day.

What do you think of using user-defined literal in C++11?
Like, `"ASCIILiteral"al`

ASCIILiteral operator "" al(const char_t* literal, size_t length) { return ASCIILiteral(literal); };

[Konstantin Tokarev]

[snip]
> > The largest reason I used this macro is that this does not allocate any
> > memory even if we use it in bmalloc's AsyncTask.
> > I don't like allocating memory dynamically in the malloc implementation.

You can also do it with static allocation, e.g. limit max size of thread name to e.g. 100 and use preallocated buffer for concatenation.

> 
> Or, another way is just using such a macro for bmalloc and use
> `ThreadName("longName", "shortName")` elsewhere.

I still think it would be better to have 2 const char* arguments in createThread, will save some punctuation.

> I think it's ok.

[Yusuke Suzuki]

(In reply to comment #27)
> [snip]
> > > The largest reason I used this macro is that this does not allocate any
> > > memory even if we use it in bmalloc's AsyncTask.
> > > I don't like allocating memory dynamically in the malloc implementation.
> 
> You can also do it with static allocation, e.g. limit max size of thread
> name to e.g. 100 and use preallocated buffer for concatenation.

Right, but it introduces the additional limitation that cannot be checked at the compile time.

> 
> > 
> > Or, another way is just using such a macro for bmalloc and use
> > `ThreadName("longName", "shortName")` elsewhere.
> 
> I still think it would be better to have 2 const char* arguments in
> createThread, will save some punctuation.

Do you mean passing 2 const char* arguments instead of passing one ThreadName?
I think the efficiency is the same since the current ThreadName always selects & holds one of two names when constructing it.

> 
> > I think it's ok.

[Yusuke Suzuki]

(In reply to comment #25)
> Comment on attachment 298354 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=298354&action=review
> 
> I would like to review the set of revised thread names separately from the
> patch that introduces all of them. Just a list. Maybe even on webkit-dev.

OK, I'll upload the patch that just introduces the mechanism.
And later, I'll upload the patch that changes the thread names.

> 
> I think all the threads that are for JavaScriptCore need to have one of
> "JavaScript", "JavaScriptCore", or "JS" in their names for clarity. Maybe
> even "JavaScriptCore:" instead of "WebKit:", but maybe "WebKit: JS xxx"
> would be enough context.

Sounds nice.

> 
> I think we should bias a lot more toward an approach that lets us write
> thread names that are a single string except in the rare cases where we
> can’t do that. As discussed elsewhere, we should be willing to do a little
> runtime work to prepend the "WebKit: " prefix to the long time, to strip
> spaces to make the short name. We might event do something exotic like
> choosing a meta-character to say "start/end of short name" if a lot of short
> names are substrings (minus spaces) of the long string.
> 
> Then we would only provide a separate short string in the rare case where
> none of those tricks will work. One downside to my ideas is that we would
> not know at compile time if the short name is short enough, but otherwise, I
> think those ideas will work well.

I think just using the `longName` and `shortName` is good for the first implementation. Once we ensured that almost all the names are representable by one name, we just implement the transformation mechanism.

One good thing: we can still check the length of the short names if we implement the transforming function (or counting function) in constexpr function form.

[Konstantin Tokarev]

(In reply to comment #28)
> (In reply to comment #27)
[snip]
> > 
> > You can also do it with static allocation, e.g. limit max size of thread
> > name to e.g. 100 and use preallocated buffer for concatenation.
> 
> Right, but it introduces the additional limitation that cannot be checked at
> the compile time.

You can get literal length at compile time by using template function with const char[]-reference argument, like what equalLettersIgnoringASCIICase does, at the cost of some code bloat.

> 
> > 
> > > 
> > > Or, another way is just using such a macro for bmalloc and use
> > > `ThreadName("longName", "shortName")` elsewhere.
> > 
> > I still think it would be better to have 2 const char* arguments in
> > createThread, will save some punctuation.
> 
> Do you mean passing 2 const char* arguments instead of passing one
> ThreadName?
> I think the efficiency is the same since the current ThreadName always
> selects & holds one of two names when constructing it.

I'm not after efficiency here, just wanting to reduce some punctuational noise in common case ({"longName", "shortName"} -> "longName", "shortName"). Though it's just my preference, others might not like it.

[Yusuke Suzuki]

Created attachment 298375 [details]
Patch

[Konstantin Tokarev]

I think you should better use { "x", "y" } instead of ThreadName { "x", "y" },

[Yusuke Suzuki]

(In reply to comment #32)
> I think you should better use { "x", "y" } instead of ThreadName { "x", "y"
> },


OK. Changed.

[Yusuke Suzuki]

Created attachment 298376 [details]
Patch

Fix build failure

[Yusuke Suzuki]

Created attachment 298377 [details]
Patch

Fix build failure

[Yusuke Suzuki]

Created attachment 298378 [details]
Patch

Fix build failure

[Yusuke Suzuki]

Created attachment 298583 [details]
Patch

[Yusuke Suzuki]

Created attachment 298585 [details]
Patch

[Yusuke Suzuki]

OK, ready :)

[Darin Adler]

Comment on attachment 298585 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=298585&action=review

Mechanism looks fine, but I think we should review the thread names first. Can we compile all the thread names into a list? I’d like to review the thread names in a context outside the whole patch. Sorry to suggest this, but maybe the thread names should be reviewed on webkit-dev?

> Source/WTF/wtf/ThreadName.h:44
> +#if OS(LINUX)
> +        static_assert((shortNameCount - 1) <= (16 - 1), "");
> +#else
> +        static_assert((stringLength("WebKit: ") + (shortNameCount - 1)) <= (32 - 1), "");
> +#endif

I think we want these static assertions to be done all the time, not just when compiling for Linux or Windows. I don’t want to break the Linux or Windows build by changing a thread name and find out only when we compile for that platform.

> Source/WTF/wtf/ThreadName.h:69
> +    template<unsigned stringCount>
> +    static constexpr unsigned stringLength(const char (&)[stringCount])
> +    {
> +        return stringCount - 1;
> +    }

Seems peculiar to have this as a member of this class. Seems of more general usefulness.

[Darin Adler]

Comment on attachment 298585 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=298585&action=review

> Source/WTF/wtf/ThreadName.h:33
> +class ThreadName {

I suggest adding a single argument constructor for names short enough to be be used as both the short name and the long name. Unless that never comes up in our thread names. I suppose I will find out when we review the thread names.

[Yusuke Suzuki]

Comment on attachment 298585 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=298585&action=review

What do you think of reviewing the thread names in the different patch?
In this patch, I just introduced the mechanism, and I didn't change thread names for macOS case.
For short names, I defined it, but I think this is ok because,

1. These names will be changed in the subsequent patch that changes all the thread names.
2. In Linux, threads are not named until recently.

>> Source/WTF/wtf/ThreadName.h:33
>> +class ThreadName {
> 
> I suggest adding a single argument constructor for names short enough to be be used as both the short name and the long name. Unless that never comes up in our thread names. I suppose I will find out when we review the thread names.

OK, added.

>> Source/WTF/wtf/ThreadName.h:44
>> +#endif
> 
> I think we want these static assertions to be done all the time, not just when compiling for Linux or Windows. I don’t want to break the Linux or Windows build by changing a thread name and find out only when we compile for that platform.

Sounds nice. We should always do that for short names!

>> Source/WTF/wtf/ThreadName.h:69
>> +    }
> 
> Seems peculiar to have this as a member of this class. Seems of more general usefulness.

Sounds nice. I think moving it to StdLibExtras.h is nice because these functions can be used for char*.
For example, if we define it in WTFString.h, we cannot use it for String low level libraries like StringHasher.

[Yusuke Suzuki]

(In reply to comment #33)
> (In reply to comment #32)
> > I think you should better use { "x", "y" } instead of ThreadName { "x", "y"
> > },
> 
> 
> OK. Changed.

After considering about it, I think `ThreadName { }` is better since it is clearer in the caller side.
And after landing this patch, we can easily extract ThreadNames by grepping it if we use ThreadName { } form.

[Yusuke Suzuki]

Created attachment 298864 [details]
Patch

[Yusuke Suzuki]

And all the thread names.
https://gist.github.com/Constellation/5499e6a8f1d1e5e995be814a64b439de

[Darin Adler]

Comment on attachment 298864 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=298864&action=review

I think this does change the thread names on macOS and iOS, by adding the "WebKit: " prefix that I asked for. I am not so happy about adding that to all the names without removing the "org.webkit.", "com.apple.WebKit.", and "WebCore: " prefixes that we have in various thread names already. I wouldn’t really be happy with that making its way into a WebKit release on macOS and iOS. Because of that I am not saying review-, but that is the only reason.

> Source/WTF/wtf/Hasher.h:4
> - *
> - * This library is free software; you can redistribute it and/or
> - * modify it under the terms of the GNU Library General Public
> + * * This library is free software; you can redistribute it and/or * modify it under the terms of the GNU Library General Public

Mandatory: Please revert this accidental change before landing anything.

> Source/WTF/wtf/StdLibExtras.h:415
> +template<typename CharType>
> +static constexpr unsigned stringLiteralLength(const CharType* characters)
> +{
> +    return (characters[0] == '\0') ? 0 : 1 + stringLiteralLength(characters + 1);
> +}

Looks good for use on actual constant expressions.

But if I accidentally call this on a const char* that is not a string literal, what will happen? I think we want a compilation error, but is that what we will get?

> Source/WTF/wtf/ThreadName.h:2
> + * Copyright (C) 2015-2016 Yusuke Suzuki <[e-mail]>.

Did you really publish part of this in 2015? Normally we think of checking something into the open source project as “publication” and so use that when deciding on copyright dates. Maybe just 2017 would be what you want here.

> Source/WTF/wtf/ThreadName.h:33
> +class ThreadName {

Important: This header needs a comment explaining the policy it intends to implement. Something roughly like this, I think:

- We define a short name that is 16 or fewer characters.
- On Windows, the short name is used instead, with the WebKit prefix added. This falls below the 32-character limit on thread names on Windows.
- On Linux, the short name is used without the WebKit prefix. This falls below the 16-character limit on thread names on Linux.
- Otherwise, the long name is used with a WebKit prefix added.

But my numbers about the thread name limits could be wrong.

> Source/WTF/wtf/ThreadName.h:39
> +        static_assert((shortNameCount - 1) <= (16 - 1), "");

Optional: Could we put something like "thread name is too long" in there instead of ""?

Optional: Is there an easy way to get this assertion in all three cases without writing it out three different times?

Optional: Why is this written with the two "-1" in two sides of the expression? Seems peculiar. Since shortNameCount is actually the length of the string plus one, I understand why the left "-1" is there, to turn char array size into the string length, but no idea why "<= 16-1" is the correct thing to assert. The parentheses also look excessive to my eyes. I am used to numeric expressions on either side of a relational operator and operator precedence is quite clear in that case; the parentheses make me wonder if something different and special is going on and I don’t think there is anything.

> Source/WTF/wtf/ThreadName.h:70
> +        constexpr unsigned prefixLength = stringLiteralLength("WebKit: ");

Optional: Reading this code, I just remembered that clang and gcc constant fold the strlen function in cases like this, and turn its result into a compile time constant if passed a string literal. That might mean that stringLiteralLength is less valuable than we previously thought: it’s a workaround for the lack of that feature in the compiler on Windows, and possibly it’s a way to assert that something is a literal. Might not be needed at all. But also, see below, not sure we need to optimize so much.

It’s also strange that we can’t use the constexpr function above for the short name assertion, instead just hard-coding in the "-1". Would be good there for clarity, I think.

Maybe we just don’t need it at all.

> Source/WTF/wtf/ThreadName.h:73
> +        CString string = CString::newUninitialized(prefixLength + contentLength, buffer);

Optional: Probably cleaner to add a helper function or constructor for CString that takes two "const char*". Even though I probably would write it this way, it’s kind of annoying to have this low level string manipulation code here and we’re probably going overboard optimizing it, too. In fact, there’s no good reason to inline this function or the name function; if we had a cpp file that went with this header I would say we should put both of these functions in there. Would be nice if the #if OS(LINUX) in the name() function wasn’t in the header and the class definition was a little cleaner.

It’s nice that we inline the constructor, so we can get the compile time assertion, along with the completely trivial pointer construction.

> Source/WTF/wtf/ThreadName.h:74
> +        memcpy(buffer, "WebKit: ", prefixLength);

Optional: Not so elegant to repeat the prefix here and also above. Can we structure this so it appears only once?

> Source/WTF/wtf/ThreadName.h:76
> +        buffer += prefixLength;
> +        memcpy(buffer, content, contentLength);

Optional: Would be better without +=; just:

    memcpy(buffer + prefixLength, content, contentLength);

> Source/WTF/wtf/ThreadingPthreads.cpp:199
> +    auto name = threadName.name();
> +    pthread_setname_np(name.data());

No need for a local variable here. This works fine and reads fine too:

    pthread_setname_np(threadName.name().data());

> Source/WTF/wtf/ThreadingPthreads.cpp:202
> +    auto name = threadName.name();
> +    prctl(PR_SET_NAME, name.data());

No need for a local variable here. This works fine and reads fine too:

    prctl(PR_SET_NAME, threadName.name().data());

> Source/WTF/wtf/ThreadingWin.cpp:133
> +    auto name = threadName.name();
>      THREADNAME_INFO info;
>      info.dwType = 0x1000;
> -    info.szName = normalizeThreadName(szThreadName);
> +    info.szName = name.data();

Same comment applies here too.