Bug 224752 – WebGL GPU Process implementation should use thread safety annotations

• Status: RESOLVED
• Resolution: FIXED
• Component: WebGL

[Kimmo Kinnunen]

WebGL GPU Process implementation should use thread safety annotations
There are few locks held.

[Kimmo Kinnunen]

Created attachment 426411 [details]
Patch

[Kenneth Russell]

Comment on attachment 426411 [details]
Patch

Having once used a similar mechanism for the traversal of the JavaScript wrappers for WebGL objects, this looks good. r+

[EWS]

Committed r276300 (236782@main): <https://commits.webkit.org/236782@main>

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

[Darin Adler]

Comment on attachment 426411 [details]
Patch

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

> Source/WebKit/GPUProcess/graphics/RemoteGraphicsContextGL.cpp:210
> -        auto locker = holdLock(mutex);
> +        Locker locker { mutex };

I’m wondering about these changes: Why doesn’t Locker work with holdLock? I would have expected maybe a change in the function name, not a change in the coding style.

[Kimmo Kinnunen]

(In reply to Darin Adler from comment #4)
> > Source/WebKit/GPUProcess/graphics/RemoteGraphicsContextGL.cpp:210
> > -        auto locker = holdLock(mutex);
> > +        Locker locker { mutex };
> 
> I’m wondering about these changes: Why doesn’t Locker work with holdLock? I
> would have expected maybe a change in the function name, not a change in the
> coding style.


This was mentioned (at least I tried) in the original thread safety analysis implementation commit:
holdLock needs more expressive Locker, e.g. one that can be moved.

The thread safety analysis does not support moves, as far as I understand. I think it has to do with lexical nature of the analysis: it cannot analyse conditional codepaths 
nor aliasing.

Locker<CheckedLock> is "different type" to Locker<Lock>, less expressive.

If I understand correctly based on the commit timestamps, holdLock pattern is a workaround from pre-c++17 lack of class template type deduction. I don't think in simple use it has any benefits over just using the simpler invocation the constructor. (unique_ptr vs make_unique, pair vs make_pair, ....) 

In my eyes both are cognitively equally taxing, but that's of course in the eye of the beholder. Of course it's bad to have two styles, but I'd hope ultimately the CheckedLock would become "Lock" and the locking pattern would be universally "Locker locker { .. }", as that should be simpler.(In reply to Darin Adler from comment #4)

[Kimmo Kinnunen]

(In reply to Darin Adler from comment #4)
> > -        auto locker = holdLock(mutex);
> > +        Locker locker { mutex };
> I’m wondering about these changes: Why doesn’t Locker work with holdLock?

And to clarify:
holdLock(T&) works with Locker, it returns Locker<T>.

Before, the `locker` was of type Locker<Lock>.
After, the `locker` is of type Locker<CheckedLock>.


holdLock does not work with CheckedLock, since it would need to construct Locker<CheckedLock> and return it, which involves move construction to make sense.

[Darin Adler]

Understood. You can’t move a Locker<CheckedLock>.

[Darin Adler]

I agree that the construction form is just fine, now that we can use deduction and even deduction guides so we can write just "Locker" and not "Locker<CheckedLock>". We can deprecate and delete holdLock now that this is possible. We can do it slowly and "naturally" or even do it globally at some point since it should be mechanical, easy, and safe.

[Ling Ho]

rdar://76892996