Bug 224752 - WebGL GPU Process implementation should use thread safety annotations
Summary: WebGL GPU Process implementation should use thread safety annotations
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebGL (show other bugs)
Version: WebKit Local Build
Hardware: Unspecified Unspecified
: P2 Enhancement
Assignee: Kimmo Kinnunen
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-04-19 05:43 PDT by Kimmo Kinnunen
Modified: 2021-04-23 02:52 PDT (History)
6 users (show)

See Also:


Attachments
Patch (10.13 KB, patch)
2021-04-19 06:14 PDT, Kimmo Kinnunen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kimmo Kinnunen 2021-04-19 05:43:53 PDT
WebGL GPU Process implementation should use thread safety annotations
There are few locks held.
Comment 1 Kimmo Kinnunen 2021-04-19 06:14:20 PDT
Created attachment 426411 [details]
Patch
Comment 2 Kenneth Russell 2021-04-19 14:15:39 PDT
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+
Comment 3 EWS 2021-04-20 04:29:40 PDT
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].
Comment 4 Darin Adler 2021-04-20 07:41:35 PDT
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.
Comment 5 Kimmo Kinnunen 2021-04-20 10:56:18 PDT
(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)
Comment 6 Kimmo Kinnunen 2021-04-20 11:03:08 PDT
(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.
Comment 7 Darin Adler 2021-04-20 11:05:52 PDT
Understood. You can’t move a Locker<CheckedLock>.
Comment 8 Darin Adler 2021-04-20 11:07:15 PDT
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.
Comment 9 Ling Ho 2021-04-23 02:52:45 PDT
rdar://76892996