Summary: | [WTF] DataMutex: Add runUnlocked() | ||||||
---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Alicia Boya García <aboya> | ||||
Component: | Web Template Framework | Assignee: | Alicia Boya García <aboya> | ||||
Status: | RESOLVED FIXED | ||||||
Severity: | Normal | CC: | benjamin, calvaris, cdumez, cmarcelo, cturner, ews-watchlist, fpizlo, webkit-bug-importer, ysuzuki | ||||
Priority: | P2 | Keywords: | InRadar | ||||
Version: | WebKit Nightly Build | ||||||
Hardware: | Unspecified | ||||||
OS: | Unspecified | ||||||
Attachments: |
|
Description
Alicia Boya García
2020-03-31 08:20:16 PDT
Created attachment 395042 [details]
Patch
Comment on attachment 395042 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=395042&action=review > Tools/TestWebKitAPI/Tests/WTF/DataMutex.cpp:30 > +#define ENABLE_DATA_MUTEX_CHECKS 1 :) > Tools/TestWebKitAPI/Tests/WTF/DataMutex.cpp:52 > + wrapper.runUnlocked([mutex]() { I don't have strong feelings here, but runUnlocked seems like it could encourage a smell. Locked scopes with lots of little unlock/lock pairs are nasty. Hard to say because I don't see real cases in your patch. wrapper.runUnlockedIf(condition, block) could be useful as well per your ChangeLog. Comment on attachment 395042 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=395042&action=review >> Tools/TestWebKitAPI/Tests/WTF/DataMutex.cpp:52 >> + wrapper.runUnlocked([mutex]() { > > I don't have strong feelings here, but runUnlocked seems like it could encourage a smell. Locked scopes with lots of little unlock/lock pairs are nasty. Hard to say because I don't see real cases in your patch. > > wrapper.runUnlockedIf(condition, block) could be useful as well per your ChangeLog. The need arises. The point is to make the smell noticeable and easily auditable. Because of the lambda capture, it's easy to see what data is used during the unlocked section. And there are still checks to avoid the most obvious accidental abuses. runUnlockedIf() sounds like a useful shorthand, but in practice, it's not more ergonomical. The reason for that is that very often after returning from the unlocked section you'll need to check that you still need to proceed with the original code (think GStreamer flushes for example). Here is an example of an usage of runUnlocked() in WebKitWebSrc create() function, so you get an idea of the scenarios where it comes handy: if (members->pendingCaps) { GST_DEBUG_OBJECT(src, "Setting caps: %" GST_PTR_FORMAT, members->pendingCaps.get()); members.runUnlocked([&baseSrc, caps = members->pendingCaps.leakRef()]() { gst_base_src_set_caps(baseSrc, caps); }); if (members->isFlushing) return GST_FLOW_FLUSHING; } (In reply to Alicia Boya García from comment #3) > Here is an example of an usage of runUnlocked() in WebKitWebSrc create() > function, so you get an idea of the scenarios where it comes handy: Makes sense, thanks. Informal r+ from me. Committed r259879: <https://trac.webkit.org/changeset/259879> All reviewed patches have been landed. Closing bug and clearing flags on attachment 395042 [details]. |