Bug 166684 – [bmalloc] Name Scavenger thread "bmalloc scavenger"

• Status: RESOLVED
• Resolution: FIXED
• Component: bmalloc

[Yusuke Suzuki]

...

[Yusuke Suzuki]

Created attachment 298019 [details]
Patch

[Darin Adler]

Comment on attachment 298019 [details]
Patch

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

Seems a nice refinement to add thread names to these threads. Slightly sad that we have to do the per-platform conditionals separately in bmalloc given we already have those in WTF, but not that big a deal I guess.

I’m not going to say review+ because I am not sure these thread names are good enough.

> Source/bmalloc/bmalloc/AsyncTask.h:46
> +    AsyncTask(Object&, const Function&, const char* threadName = "bm-task");

I think we should consider clearer more descriptive names for these threads. Keep in mind that these threads will exist in lots of different apps that just happen to be using WebKit or JavaScriptCore, and people won’t know what "bm" stands for.

> Source/bmalloc/bmalloc/AsyncTask.h:82
> +    // On Linux system, the max thread name length is 15.

Sadly, I think this limit is motivating us to chose thread names that aren’t as descriptive as I would like on macOS and iOS.

> Source/bmalloc/bmalloc/BPlatform.h:51
> +#if defined(WIN32) || defined(_WIN32)
> +#define BOS_WINDOWS 1
> +#endif

Doesn’t seem necessary to add this since we are not yet using it.

> Source/bmalloc/bmalloc/Heap.cpp:39
> +    , m_scavenger(*this, &Heap::concurrentScavenge, "bm-scavenger")

Same comment about thread name being too brief.

[Geoffrey Garen]

Comment on attachment 298019 [details]
Patch

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

>> Source/bmalloc/bmalloc/AsyncTask.h:46
>> +    AsyncTask(Object&, const Function&, const char* threadName = "bm-task");
> 
> I think we should consider clearer more descriptive names for these threads. Keep in mind that these threads will exist in lots of different apps that just happen to be using WebKit or JavaScriptCore, and people won’t know what "bm" stands for.

I would just called this "name" rather than threadName. AsyncTask tries to abstract away the fact that its implementation is a thread. A queue or some other implementation would be OK too.

I don't think we want a default name -- empty is probably a better default.

> Source/bmalloc/bmalloc/AsyncTask.h:83
> +    BASSERT(strlen(threadName) <= 15);

Maybe we should just allow Linux to truncate to "bmalloc scaveng", so other platforms can use a good name.

> Source/bmalloc/bmalloc/AsyncTask.h:125
> +#if BOS(DARWIN)
> +    pthread_setname_np(asyncTask->threadName());
> +#elif BOS(LINUX)
> +    prctl(PR_SET_NAME, asyncTask->threadName());
> +#endif

I think Linux supports pthread_setname_np -- maybe we can simplify by using it there too.

>> Source/bmalloc/bmalloc/Heap.cpp:39
>> +    , m_scavenger(*this, &Heap::concurrentScavenge, "bm-scavenger")
> 
> Same comment about thread name being too brief.

I'd call this "bmalloc scavenger".

[Michael Catanzaro]

(In reply to comment #3)
> > Source/bmalloc/bmalloc/AsyncTask.h:125
> > +#if BOS(DARWIN)
> > +    pthread_setname_np(asyncTask->threadName());
> > +#elif BOS(LINUX)
> > +    prctl(PR_SET_NAME, asyncTask->threadName());
> > +#endif
> 
> I think Linux supports pthread_setname_np -- maybe we can simplify by using
> it there too.

Hm, you're right. There is a behavior difference though. prctl will silently truncate the thread name if it's longer than 16 characters. pthread_setname_np will instead fail and set ERANGE.

[Yusuke Suzuki]

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

[Michael Catanzaro]

Um, Yusuke, I remember you had done quite some work on thread naming. Is this obsolete?

[Yusuke Suzuki]

(In reply to Michael Catanzaro from comment #6)
> Um, Yusuke, I remember you had done quite some work on thread naming. Is
> this obsolete?

I'll restart this bmalloc change again.

[Yusuke Suzuki]

Comment on attachment 298019 [details]
Patch

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

>>> Source/bmalloc/bmalloc/AsyncTask.h:46
>>> +    AsyncTask(Object&, const Function&, const char* threadName = "bm-task");
>> 
>> I think we should consider clearer more descriptive names for these threads. Keep in mind that these threads will exist in lots of different apps that just happen to be using WebKit or JavaScriptCore, and people won’t know what "bm" stands for.
> 
> I would just called this "name" rather than threadName. AsyncTask tries to abstract away the fact that its implementation is a thread. A queue or some other implementation would be OK too.
> 
> I don't think we want a default name -- empty is probably a better default.

OK, use `bmalloc scavenger`.

>> Source/bmalloc/bmalloc/AsyncTask.h:82
>> +    // On Linux system, the max thread name length is 15.
> 
> Sadly, I think this limit is motivating us to chose thread names that aren’t as descriptive as I would like on macOS and iOS.

Use untruncated name for macOS. In Linux, we truncate the name into 16 length.

>> Source/bmalloc/bmalloc/AsyncTask.h:83
>> +    BASSERT(strlen(threadName) <= 15);
> 
> Maybe we should just allow Linux to truncate to "bmalloc scaveng", so other platforms can use a good name.

Done.

>> Source/bmalloc/bmalloc/AsyncTask.h:125
>> +#endif
> 
> I think Linux supports pthread_setname_np -- maybe we can simplify by using it there too.

Right. But funny thing is that "pthread_setname_np" signature is different in Linux and Darwin. So still ifdef is necessary.
But we can use pthread_setname_np for both. Fixed.

>> Source/bmalloc/bmalloc/BPlatform.h:51
>> +#endif
> 
> Doesn’t seem necessary to add this since we are not yet using it.

Thanks. This change is discarded since they are already introduced in bmalloc ToT in the other change.

>>> Source/bmalloc/bmalloc/Heap.cpp:39
>>> +    , m_scavenger(*this, &Heap::concurrentScavenge, "bm-scavenger")
>> 
>> Same comment about thread name being too brief.
> 
> I'd call this "bmalloc scavenger".

OK, let's use the name "bmalloc scavenger" for now in both Linux and macOS.

[Yusuke Suzuki]

Created attachment 337574 [details]
Patch

[EWS Watchlist]

Comment on attachment 337574 [details]
Patch

Attachment 337574 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: http://webkit-queues.webkit.org/results/7262535

New failing tests:
imported/w3c/web-platform-tests/workers/name-property.html

[EWS Watchlist]

Created attachment 337580 [details]
Archive of layout-test-results from ews122 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews122  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.4

[Yusuke Suzuki]

(In reply to Build Bot from comment #10)
> Comment on attachment 337574 [details]
> Patch
> 
> Attachment 337574 [details] did not pass ios-sim-ews (ios-simulator-wk2):
> Output: http://webkit-queues.webkit.org/results/7262535
> 
> New failing tests:
> imported/w3c/web-platform-tests/workers/name-property.html

The results seem not related to this change. It is not related to bmalloc scavenger thread.

[Saam Barati]

Comment on attachment 337574 [details]
Patch

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

> Source/bmalloc/bmalloc/Scavenger.h:86
> +    void setName(const char*);

I'd call this setThreadName or something like that.

[Yusuke Suzuki]

Comment on attachment 337574 [details]
Patch

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

>> Source/bmalloc/bmalloc/Scavenger.h:86
>> +    void setName(const char*);
> 
> I'd call this setThreadName or something like that.

OK, fixed.

[Yusuke Suzuki]

Committed r230474: <https://trac.webkit.org/changeset/230474>

[Radar WebKit Bug Importer]

<rdar://problem/39308005>

[Darin Adler]

Still not thrilled about this thread name on Apple’s platforms like iOS and macOS. Might want to improve the situation there with additional changes. Thread names are seen in crash logs by many who are not expert on the internals of WebKit, and the name "bmalloc" might not make anyone think of JavaScriptCore or WebKit: those are the public names of the frameworks that make use of bmalloc.

[Yusuke Suzuki]

(In reply to Darin Adler from comment #17)
> Still not thrilled about this thread name on Apple’s platforms like iOS and
> macOS. Might want to improve the situation there with additional changes.
> Thread names are seen in crash logs by many who are not expert on the
> internals of WebKit, and the name "bmalloc" might not make anyone think of
> JavaScriptCore or WebKit: those are the public names of the frameworks that
> make use of bmalloc.

maybe having two names for linux and others is the best way. if we add more 
words to the name, the name becomes too difficult to understand in linux.
“JavaScriptCore bmalloc scavenger” / “bm-scavenger” would be better. I’ll land the follow up for that.

[Yusuke Suzuki]

Committed r230897: <https://trac.webkit.org/changeset/230897>