Bug 236832 – Thread safety analysis to assert "code is run sequentially" is not useful when code is mainly run with WorkQueues

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Template Framework

[Kimmo Kinnunen]

Thread safety analysis is not useful when code is mainly run with WorkQueues

Currently we have assertion "this code runs in a specific thread".

WorkQueue runnables might run in arbitrary thread, but still serially in a specific "work queue".

On Cocoa, Dispatch will switch the threads.

[Kimmo Kinnunen]

Created attachment 452502 [details]
Patch

[Kimmo Kinnunen]

Created attachment 452503 [details]
Patch

[Jean-Yves Avenard [:jya]]

Comment on attachment 452503 [details]
Patch

this is great. The name Sequence however is a bit confusing.

[Chris Dumez]

Comment on attachment 452503 [details]
Patch

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

I like the idea, this allows for a bit more fine grained threading checks (we usually just check main thread vs background thread only, because of work queues).

> Source/WTF/wtf/ThreadAssertions.h:49
> +class WTF_CAPABILITY_LOCK SequenceAssertion {

For what it's worth I'm really not a fan of the SequenceAssertion naming. I have trouble making the connection between thread / work queue and the term "sequence".
I understand why you chose this name of course but I still think it is not clear enough.

Sadly it is hard for me to come up with a better name here. Maybe "ThreadingAssertion"? "ThreadOrWorkQueueAssertion"?

I added a few people in cc who often have good naming proposals.

> Source/WTF/wtf/ThreadAssertions.h:63
> +    uint32_t m_uid { currentSequenceID() };

For example, I think currentThreadOrWorkQueueID() would read better than currentSequenceID() here, even if it is a bit of a mouthful.

> Source/WTF/wtf/Threading.h:107
> +class WTF_CAPABILITY_LOCK Thread : public ThreadSafeRefCounted<Thread> {

I admit I don't understand why we're claiming the Thread class is a lock.

> Source/WTF/wtf/Threading.h:436
> +inline void assertIsCurrent(const Thread& thread) WTF_ASSERTS_ACQUIRED_LOCK(thread)

And why this "acquires" the lock.

> Source/WTF/wtf/WorkQueue.h:92
> +class WTF_CAPABILITY_LOCK WorkQueue : public WorkQueueBase {

Same comment about claiming this is a lock.

> Source/WTF/wtf/WorkQueue.h:116
> +    WTF_EXPORT_PRIVATE void assertIsCurrent() const;

I think ideally, these assertIsCurrent() function would not be behind a ASSERT_ENABLED flag because this will require every call site to check for the flag and it will look ugly.
I'd rather we define the function unconditionally and only conditionalize its implementation (making sure that its implementation is both empty and inlined when !ASSERT_ENABLED.

> Source/WTF/wtf/cocoa/WorkQueueCocoa.cpp:94
> +    dispatch_queue_set_specific(m_dispatchQueue.get(), &Thread::s_uid, reinterpret_cast<void*>(++Thread::s_uid), nullptr);

Nice trick :)

[Chris Dumez]

(In reply to Jean-Yves Avenard [:jya] from comment #3)
> Comment on attachment 452503 [details]
> Patch
> 
> this is great. The name Sequence however is a bit confusing.

ahah, I hadn't seen Jean-Yves's statement earlier but I am glad we're in agreement :)

[Kimmo Kinnunen]

(In reply to Chris Dumez from comment #4)
> > Source/WTF/wtf/Threading.h:107
> > +class WTF_CAPABILITY_LOCK Thread : public ThreadSafeRefCounted<Thread> {
> 
> I admit I don't understand why we're claiming the Thread class is a lock.

The Thread has the capability lock. Lock capability is something that has "acquire" and "release" operations of abstract something.

The lock capability of the thread means "is it current or not".


> > Source/WTF/wtf/Threading.h:436
> > +inline void assertIsCurrent(const Thread& thread) WTF_ASSERTS_ACQUIRED_LOCK(thread)
> 
> And why this "acquires" the lock.

Contrast the thing we already have:

Lock& myLock = ...;
assertIsHeld(myLock);
// Compiler knows the myLock is held.

.. to the thing we should have (this patch implements this):

Thread&  myThread = ...;
assertIsCurrent(myThread);
// The compiler knows the myThread is current.

We already can say:
This code compiles only if the compiler can prove that the variable being referenced is referenced with the expected mutex held.

With this patch, you can say:
this code compiles only if the compiler can prove that the variable being referenced is referenced in the expected thread.

The latter is useful in scenarios where the variables and functions are not protected by a lock, they're protected by the logical choice that they're only accessed sequentially, e.g. in one sequence, e.g. in one work queue or thread.


> > Source/WTF/wtf/WorkQueue.h:92
> > +class WTF_CAPABILITY_LOCK WorkQueue : public WorkQueueBase {
> 
> Same comment about claiming this is a lock.
> 
> > Source/WTF/wtf/WorkQueue.h:116
> > +    WTF_EXPORT_PRIVATE void assertIsCurrent() const;
> 
> I think ideally, these assertIsCurrent() function would not be behind a
> ASSERT_ENABLED flag because this will require every call site to check for
> the flag and it will look ugly.

It is exactly like that. The use is:

assertIsCurrent(myThread);

assertIsCurrent(myQueue);

No ifdefs.


The function you quote is a private one. Nobody is calling that (except the implementation of assertIsCurrent();

[Kimmo Kinnunen]

For the name:
we want things to execute sequentially as opposed to parallelly.
E.g. in sequence, as opposed to in parallel.

https://www.dictionary.com/browse/sequence
> the following of one thing after another; succession.

FWIW, while it is a bit of a stretch to claim we might have another abstraction for parallelism to support, I find ConcreteAOrConcreteB(OrConcreteC) a bit clumsy.

[Kimmo Kinnunen]

(IIRC in chromium it's called a sequence.)

[David Kilzer (:ddkilzer)]

(In reply to Kimmo Kinnunen from comment #8)
> (IIRC in chromium it's called a sequence.)

I haven't heard the term "sequence" used before for this, but I suppose that term is less overloaded than "serial" (which I generally think of as the opposite of "parallel").

I suppose "sequence" here means a "serial execution context"--some mechanism that forces code to execute serially (e.g., not parallel, or "in sequence"), but I'm not sure SerialAssertion or SerialExecutionAssertion (or SerialExecutionContextAssertion) is any better than SequenceAssertion.  Maybe SequentialExecutionAssertion?

Just writing this down in case it causes someone else to think of a better name.

[Ryosuke Niwa]

Comment on attachment 452503 [details]
Patch

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

>> Source/WTF/wtf/ThreadAssertions.h:49
>> +class WTF_CAPABILITY_LOCK SequenceAssertion {
> 
> For what it's worth I'm really not a fan of the SequenceAssertion naming. I have trouble making the connection between thread / work queue and the term "sequence".
> I understand why you chose this name of course but I still think it is not clear enough.
> 
> Sadly it is hard for me to come up with a better name here. Maybe "ThreadingAssertion"? "ThreadOrWorkQueueAssertion"?
> 
> I added a few people in cc who often have good naming proposals.

How about NoParallelismAssertion? or ParallelismDisallowedAssertion?
I like ThreadOrWorkQueueAssertion but it sort of lacks information about
what aspect of thread or work queue is being asserted.

[Chris Dumez]

(In reply to Ryosuke Niwa from comment #10)
> Comment on attachment 452503 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=452503&action=review
> 
> >> Source/WTF/wtf/ThreadAssertions.h:49
> >> +class WTF_CAPABILITY_LOCK SequenceAssertion {
> > 
> > For what it's worth I'm really not a fan of the SequenceAssertion naming. I have trouble making the connection between thread / work queue and the term "sequence".
> > I understand why you chose this name of course but I still think it is not clear enough.
> > 
> > Sadly it is hard for me to come up with a better name here. Maybe "ThreadingAssertion"? "ThreadOrWorkQueueAssertion"?
> > 
> > I added a few people in cc who often have good naming proposals.
> 
> How about NoParallelismAssertion? or ParallelismDisallowedAssertion?
> I like ThreadOrWorkQueueAssertion but it sort of lacks information about
> what aspect of thread or work queue is being asserted.

I think it is made clear by how it is used:
`assertIsCurrent(m_threadOrWorkQueueAssertion);`

[Jean-Yves Avenard [:jya]]

In https://bugs.webkit.org/show_bug.cgi?id=235353 where I wanted to assert that we were running on a particular WorkQueue I added the following

void WorkQueueBase::assertIsOnWorkQueue() const
{
#if ASSERT_ENABLED
    dispatch_assert_queue(m_dispatchQueue.get());
#endif
}

For naming,
in gecko's xpcom, they have EventQueue and SerialEventQueue that inherits from it.

A thread is a SerialEventQueue, a ConcurrentWorkQueue is an EventQueue

[Ryosuke Niwa]

(In reply to Chris Dumez from comment #11)
> (In reply to Ryosuke Niwa from comment #10)
> > Comment on attachment 452503 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=452503&action=review
> > 
> > >> Source/WTF/wtf/ThreadAssertions.h:49
> > >> +class WTF_CAPABILITY_LOCK SequenceAssertion {
> > > 
> > > For what it's worth I'm really not a fan of the SequenceAssertion naming. I have trouble making the connection between thread / work queue and the term "sequence".
> > > I understand why you chose this name of course but I still think it is not clear enough.
> > > 
> > > Sadly it is hard for me to come up with a better name here. Maybe "ThreadingAssertion"? "ThreadOrWorkQueueAssertion"?
> > > 
> > > I added a few people in cc who often have good naming proposals.
> > 
> > How about NoParallelismAssertion? or ParallelismDisallowedAssertion?
> > I like ThreadOrWorkQueueAssertion but it sort of lacks information about
> > what aspect of thread or work queue is being asserted.
> 
> I think it is made clear by how it is used:
> `assertIsCurrent(m_threadOrWorkQueueAssertion);`

This reads weird. What we really want to say is something like:
ASSERT(thread or work queue is what we expect it to be)
right? In that case, <whatever> should be thread or a work queue, not an assertion.

So perhaps this should be called something like ThreadOrWorkQueue or even ThreadOrWorkQueueHandle so that we can say:
ASSERT(m_threadOrWorkQueue.isCurrent());
or
ASSERT(m_threadOrWorkQueueHandle.isCurrent());

[Ryosuke Niwa]

Comment on attachment 452503 [details]
Patch

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

>> Source/WTF/wtf/ThreadAssertions.h:63
>> +    uint32_t m_uid { currentSequenceID() };
> 
> For example, I think currentThreadOrWorkQueueID() would read better than currentSequenceID() here, even if it is a bit of a mouthful.

Yeah, that would read a lot better.

> Source/WTF/wtf/Threading.h:441
> +inline constexpr uint32_t invalidSequenceID { 0 };

Can we use enum class instead of uint32_t so that we don't accidentally implicitly coerce it to different integer types?
e.g. enum class ThreadOrWorkQueueIdentifier : uint32_t { Invalid };

>> Source/WTF/wtf/WorkQueue.h:116
>> +    WTF_EXPORT_PRIVATE void assertIsCurrent() const;
> 
> I think ideally, these assertIsCurrent() function would not be behind a ASSERT_ENABLED flag because this will require every call site to check for the flag and it will look ugly.
> I'd rather we define the function unconditionally and only conditionalize its implementation (making sure that its implementation is both empty and inlined when !ASSERT_ENABLED.

I'd like to make this function available everywhere though.
I can definitely see cases where we want to release-assert it.

[Chris Dumez]

(In reply to Ryosuke Niwa from comment #13)
> (In reply to Chris Dumez from comment #11)
> > (In reply to Ryosuke Niwa from comment #10)
> > > Comment on attachment 452503 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=452503&action=review
> > > 
> > > >> Source/WTF/wtf/ThreadAssertions.h:49
> > > >> +class WTF_CAPABILITY_LOCK SequenceAssertion {
> > > > 
> > > > For what it's worth I'm really not a fan of the SequenceAssertion naming. I have trouble making the connection between thread / work queue and the term "sequence".
> > > > I understand why you chose this name of course but I still think it is not clear enough.
> > > > 
> > > > Sadly it is hard for me to come up with a better name here. Maybe "ThreadingAssertion"? "ThreadOrWorkQueueAssertion"?
> > > > 
> > > > I added a few people in cc who often have good naming proposals.
> > > 
> > > How about NoParallelismAssertion? or ParallelismDisallowedAssertion?
> > > I like ThreadOrWorkQueueAssertion but it sort of lacks information about
> > > what aspect of thread or work queue is being asserted.
> > 
> > I think it is made clear by how it is used:
> > `assertIsCurrent(m_threadOrWorkQueueAssertion);`
> 
> This reads weird. What we really want to say is something like:
> ASSERT(thread or work queue is what we expect it to be)
> right? In that case, <whatever> should be thread or a work queue, not an
> assertion.
> 
> So perhaps this should be called something like ThreadOrWorkQueue or even
> ThreadOrWorkQueueHandle so that we can say:
> ASSERT(m_threadOrWorkQueue.isCurrent());
> or
> ASSERT(m_threadOrWorkQueueHandle.isCurrent());

I agree with Ryosuke that using an ASSERT() would be better than the assertIsCurrent() free function. It would be a lot more consistent with current WebKit practices.

The issue with the Handle naming is that the class is question actually does embed an assertion. There is an assertion in its destructor to make sure it gets called on the same work queue / thread as the constructor.

[Jean-Yves Avenard [:jya]]

(In reply to Ryosuke Niwa from comment #13)

> This reads weird. What we really want to say is something like:
> ASSERT(thread or work queue is what we expect it to be)
> right? In that case, <whatever> should be thread or a work queue, not an
> assertion.
> 
> So perhaps this should be called something like ThreadOrWorkQueue or even
> ThreadOrWorkQueueHandle so that we can say:
> ASSERT(m_threadOrWorkQueue.isCurrent());
> or
> ASSERT(m_threadOrWorkQueueHandle.isCurrent());

Unfortunately macOS's dispatch_queues don't make it easy to achieve such semantic.
You can't retrieve what the currently running dispatch_queue is, and the only method provided to check if you are currently running code in the dispatch_queue is `dispatch_assert_queue` 
(https://developer.apple.com/documentation/dispatch/1642201-dispatch_assert_queue)

So that makes implementing WorkQueue::isCurrent() overly complicated (it's doable though, but at a cost: you could store a value in a TLS variable before each run of a task and restore the previous value after the task is run)
(gecko's TaskQueue implementation: https://searchfox.org/mozilla-central/source/xpcom/threads/TaskQueue.cpp#199-215)

[Chris Dumez]

(In reply to Jean-Yves Avenard [:jya] from comment #16)
> (In reply to Ryosuke Niwa from comment #13)
> 
> > This reads weird. What we really want to say is something like:
> > ASSERT(thread or work queue is what we expect it to be)
> > right? In that case, <whatever> should be thread or a work queue, not an
> > assertion.
> > 
> > So perhaps this should be called something like ThreadOrWorkQueue or even
> > ThreadOrWorkQueueHandle so that we can say:
> > ASSERT(m_threadOrWorkQueue.isCurrent());
> > or
> > ASSERT(m_threadOrWorkQueueHandle.isCurrent());
> 
> Unfortunately macOS's dispatch_queues don't make it easy to achieve such
> semantic.
> You can't retrieve what the currently running dispatch_queue is, and the
> only method provided to check if you are currently running code in the
> dispatch_queue is `dispatch_assert_queue` 
> (https://developer.apple.com/documentation/dispatch/1642201-
> dispatch_assert_queue)
> 
> So that makes implementing WorkQueue::isCurrent() overly complicated (it's
> doable though, but at a cost: you could store a value in a TLS variable
> before each run of a task and restore the previous value after the task is
> run)
> (gecko's TaskQueue implementation:
> https://searchfox.org/mozilla-central/source/xpcom/threads/TaskQueue.cpp#199-
> 215)

What do you mean? This patch already stores an ID in dispatch queue specific storage, and we can access it via currentSequenceID() and we can check if it is the same ID as the « handle ».

[Jean-Yves Avenard [:jya]]

(In reply to Chris Dumez from comment #17)
> What do you mean? This patch already stores an ID in dispatch queue specific
> storage, and we can access it via currentSequenceID() and we can check if it
> is the same ID as the « handle ».

Missed that part completely :)

neat trick !

[Antti Koivisto]

Comment on attachment 452503 [details]
Patch

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

>> Source/WTF/wtf/Threading.h:107
>> +class WTF_CAPABILITY_LOCK Thread : public ThreadSafeRefCounted<Thread> {
> 
> I admit I don't understand why we're claiming the Thread class is a lock.

This patch might be easier to understand if it didn't use "lock" terminology for these capabilities. Maybe there could instead be a separate WTF_CAPABILITY_QUEUE/WTF_ASSERTS_ACQUIRED_QUEUE or similar (if I understand correctly the capability names are arbitrary strings that show up in error messages)? Clang seems to recommend against calling non-mutexy things locks too in https://clang.llvm.org/docs/ThreadSafetyAnalysis.html#reference-guide:

"For historical reasons, prior versions of thread safety used macro names that were very lock-centric. These macros have since been renamed to fit a more general capability model."

[Antti Koivisto]

Comment on attachment 452503 [details]
Patch

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

>> Source/WTF/wtf/Threading.h:436
>> +inline void assertIsCurrent(const Thread& thread) WTF_ASSERTS_ACQUIRED_LOCK(thread)
> 
> And why this "acquires" the lock.

Maybe these could be called acquireQueue() or similar since acquiring the capability for the subsequent code is really the core thing it does?

("Queue" might be a decent name covering both Threads and WorkQueues. While generic the meaning should be clear from context.)

[Antti Koivisto]

> ("Queue" might be a decent name covering both Threads and WorkQueues. While
> generic the meaning should be clear from context.)

Actually I'm not sure there is value in unifying these in the first place. Threads and WorkQueues are distinct concepts. Can they just have separate capabilities?

[Kimmo Kinnunen]

Created attachment 452842 [details]
Patch

[Kimmo Kinnunen]

Thank you for spending the time!

>The name Sequence however is a bit confusing.

This discussion should be continued in bug 237026 and should not block this patch unless the small part left in this bug still bothers the reviewers.

> I admit I don't understand why we're claiming the Thread class is a lock.

This confusion is addressed in bug 237022 that this bug now blocks on.

> So perhaps this should be called something like ThreadOrWorkQueue or even  ThreadOrWorkQueueHandle so that we can say:
> ASSERT(m_threadOrWorkQueue.isCurrent());
> or
> ASSERT(m_threadOrWorkQueueHandle.isCurrent());

These are invalid suggestions that go against the exact purpose of this patch.
Thread safety analysis can not prove anything based on this.

> So that makes implementing WorkQueue::isCurrent() overly complicated 

Implementing WorkQueue::isCurrent() is possible. It is inferior to currently presented code because:
- it needs an extra instance variable that would be mostly useless
- it is the antithesis of the purpose of this work, e.g. to enable more use of the thread safety analysis. For the purposes of this work, I cannot see any correct use-case for isCurrent(). It is also addable later trivially. This work should not discuss this function unless it somehow blocks the needed functionality in this patch.

> .. ASSERT_ENABLED ...

I'd like to continue with this approach, e.g. enabling the real assertions ASSERT_ENABLED. This is to match the exact existing code we have. Should we want something different, I propose we address that in separate work.



I re-worked the patch to reduce the possible confusion.

I removed the changes that relate to the important naming discussion.

The payload of the patch is roughly 10 lines.

There's 2 tests explaining the intent should it be confusing. 

There's numerous lines of examples what are the implications.

Should you be too busy for a review of the *subject matter*, and should something slip in to trunk where you do not like some nomenclature or textual representation: we can have another bug, assigned to me, fixing that to your personal preference.

[Kimmo Kinnunen]

Created attachment 452963 [details]
Patch

[Kimmo Kinnunen]

Created attachment 452973 [details]
Patch

[Antti Koivisto]

Comment on attachment 452973 [details]
Patch

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

r=me, cool patch

> Source/WTF/wtf/Threading.h:106
> +class WTF_CAPABILITY("is current") Thread : public ThreadSafeRefCounted<Thread> {

Space here looks od but I suppose it is ok.

[Antti Koivisto]

Comment on attachment 452973 [details]
Patch

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

> Source/WebKit/GPUProcess/graphics/RemoteGraphicsContextGL.h:-150
> -    NO_UNIQUE_ADDRESS ThreadAssertion m_streamThread;

Can ThreadAssertion now be removed?

[Kimmo Kinnunen]

Comment on attachment 452973 [details]
Patch

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

>> Source/WebKit/GPUProcess/graphics/RemoteGraphicsContextGL.h:-150
>> -    NO_UNIQUE_ADDRESS ThreadAssertion m_streamThread;
> 
> Can ThreadAssertion now be removed?

No, it's still in use, although incorrectly. It's also usefulish but not really useful.
It's also reworked in the bug depending on this bug, e.g. the SequenceAssertion work.

[Kimmo Kinnunen]

(In reply to Kimmo Kinnunen from comment #28)
> Comment on attachment 452973 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=452973&action=review
> 
> >> Source/WebKit/GPUProcess/graphics/RemoteGraphicsContextGL.h:-150
> >> -    NO_UNIQUE_ADDRESS ThreadAssertion m_streamThread;
> > 
> > Can ThreadAssertion now be removed?
> 
> No, it's still in use, although incorrectly. It's also usefulish but not
> really useful.
> It's also reworked in the bug depending on this bug, e.g. the
> SequenceAssertion work.

SequenceAssertion is useful in fixing the problem that is WeakPtr `m_impl->wasConstructedOnMainThread() == isMainThread()`

[EWS]

Committed r290418 (247726@main): <https://commits.webkit.org/247726@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 452973 [details].

[Radar WebKit Bug Importer]

<rdar://problem/89405701>