[webkit-reviews] review canceled: [Bug 166684] [bmalloc] Set name for bmalloc's AsyncTask : [Attachment 298019] Patch

[bugzilla-daemon at webkit.org]

Yusuke Suzuki <utatane.tea at gmail.com> has canceled Yusuke Suzuki
<utatane.tea at gmail.com>'s request for review:
Bug 166684: [bmalloc] Set name for bmalloc's AsyncTask
https://bugs.webkit.org/show_bug.cgi?id=166684

Attachment 298019: Patch

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




--- Comment #8 from Yusuke Suzuki <utatane.tea at gmail.com> ---
Comment on attachment 298019
  --> https://bugs.webkit.org/attachment.cgi?id=298019
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.