Bug 220110

Summary: Add a helper method to WTF::MachSemaphore to wait with a timeout duration
Product: WebKit Reporter: Wenson Hsieh <wenson_hsieh>
Component: Web Template FrameworkAssignee: Wenson Hsieh <wenson_hsieh>
Status: RESOLVED FIXED    
Severity: Normal CC: benjamin, cdumez, cmarcelo, darin, ews-watchlist, ggaren, sam, webkit-bug-importer
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Bug Depends on:    
Bug Blocks: 219586    
Attachments:
Description Flags
WIP
none
Patch
sam: review-
Patch none

Wenson Hsieh
Reported 2020-12-22 19:09:58 PST
Namely, the ability to broadcast (semaphore_signal_all), and the ability to wait for a limited amount of time before giving up (semaphore_timedwait)
Attachments
WIP (2.74 KB, patch)
2020-12-23 16:30 PST, Wenson Hsieh 		no flags
DetailsFormatted DiffDiff
Patch (2.45 KB, patch)
2020-12-27 11:20 PST, Wenson Hsieh 		sam: review-
DetailsFormatted DiffDiff
Patch (2.37 KB, patch)
2020-12-27 13:22 PST, Wenson Hsieh 		no flags
DetailsFormatted DiffDiff
Show Obsolete (2) View All Add attachment proposed patch, testcase, etc.
Wenson Hsieh
Comment 1 2020-12-23 16:30:21 PST
Created attachment 416730 [details] WIP
Wenson Hsieh
Comment 2 2020-12-27 11:20:08 PST
Created attachment 416823 [details] Patch
Sam Weinig
Comment 3 2020-12-27 13:08:39 PST
Comment on attachment 416823 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=416823&action=review > Source/WTF/wtf/cocoa/MachSemaphore.cpp:62 > +void MachSemaphore::waitWithTimeout(Seconds timeout) We call this waitFor() in WTF::Semaphore and WTF::BinarySemaphore, perhaps we should keep the names the same? > Source/WTF/wtf/cocoa/MachSemaphore.cpp:64 > + auto seconds = static_cast<unsigned>(timeout.seconds()); I think you can use timeout.secondsAs<unsigned>() here and below instead of explicitly casting yourself.
Wenson Hsieh
Comment 4 2020-12-27 13:18:12 PST
Comment on attachment 416823 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=416823&action=review >> Source/WTF/wtf/cocoa/MachSemaphore.cpp:62 >> +void MachSemaphore::waitWithTimeout(Seconds timeout) > > We call this waitFor() in WTF::Semaphore and WTF::BinarySemaphore, perhaps we should keep the names the same? Sounds good — changed to waitFor(). >> Source/WTF/wtf/cocoa/MachSemaphore.cpp:64 >> + auto seconds = static_cast<unsigned>(timeout.seconds()); > > I think you can use timeout.secondsAs<unsigned>() here and below instead of explicitly casting yourself. timeout.secondsAs<unsigned>() ought to work. However, timeout.nanosecondsAs<unsigned>() won't work for anything above a few seconds. I can use timeout.nanosecondsAs<int64_t>() and then subtract against the number of nanoseconds, but I'll still need a static cast to clock_res_t around the result.
Wenson Hsieh
Comment 5 2020-12-27 13:22:20 PST
Created attachment 416825 [details] Patch
Wenson Hsieh
Comment 6 2020-12-27 22:41:30 PST
Comment on attachment 416825 [details] Patch Thanks for the review!
EWS
Comment 7 2020-12-27 22:45:19 PST
Committed r271101: <https://trac.webkit.org/changeset/271101> All reviewed patches have been landed. Closing bug and clearing flags on attachment 416825 [details].
Radar WebKit Bug Importer
Comment 8 2020-12-27 22:46:18 PST
<rdar://problem/72697931>
Darin Adler
Comment 9 2021-01-02 16:56:17 PST
Comment on attachment 416825 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=416825&action=review > Source/WTF/wtf/cocoa/MachSemaphore.cpp:65 > + auto seconds = timeout.secondsAs<unsigned>(); > + semaphore_timedwait(m_semaphore, { seconds, static_cast<clock_res_t>(timeout.nanosecondsAs<uint64_t>() - seconds * NSEC_PER_SEC) }); Seems like the "convert Seconds to a mach_timespec_t" should be a separate function; could be useful elsewhere and a good separate concept. Simpler but related to MonotonicTime::fromMachAbsoluteTime.
Note You need to log in before you can comment on or make changes to this bug.