On Mon, 24 Oct 2022 20:43:48 GMT, Alex Menkov <amen...@openjdk.org> wrote:
>> The implementation of removeThread() is currently:
>>
>>
>> static void
>> removeThread(JNIEnv *env, ThreadList *list, jthread thread)
>> {
>> ThreadNode *node;
>>
>> node = findThread(list, thread);
>> if (node != NULL) {
>> removeNode(list, node);
>> clearThread(env, node);
>> }
>> }
>>
>> However, currently all callers already have the ThreadNode*, so they end up
>> calling like the following:
>>
>> ` removeThread(env, list, node->thread);`
>>
>> So we go from a ThreadNode* to a jthread, only to do a findThread() to get
>> the ThreadNode* again. Also, the list is stored in the ThreadNode.
>> removeThread() can instead be implemented as:
>>
>>
>> static void
>> removeThread(JNIEnv *env, ThreadNode *node)
>> {
>> JDI_ASSERT(node != NULL);
>> removeNode(node->list, node);
>> clearThread(env, node);
>> }
>>
>>
>> This is faster, although not by as much as you might think. TLS is used to
>> map a jthread to a ThreadNode*, so the findThread() call is actually already
>> pretty fast. The exception is when dealing with the otherThreads list. These
>> threads have not yet been started, so TLS cannot be used, but it is rare for
>> threads to appear on this list. Still, this is a good cleanup to do, and
>> should be a bit faster.
>>
>> This cleanup was initially implemented as part of the work for
>> [JDK-8295376](https://bugs.openjdk.org/browse/JDK-8295376), but is being
>> split out.
>
> src/jdk.jdwp.agent/share/native/libjdwp/threadControl.c line 322:
>
>> 320: ThreadNode *next;
>> 321:
>> 322: JDI_ASSERT(list == node->list);
>
> "list" argument is redundant
> I suggest to drop it and replace this assert with
> `ThreadList *list == node->list;`
I was almost to suggest the same.
-------------
PR: https://git.openjdk.org/jdk/pull/10828
