Bug 220110 - Add a helper method to WTF::MachSemaphore to wait with a timeout duration
Summary: Add a helper method to WTF::MachSemaphore to wait with a timeout duration
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Template Framework (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Wenson Hsieh
URL:
Keywords: InRadar
Depends on:
Blocks: 219586
  Show dependency treegraph
 
Reported: 2020-12-22 19:09 PST by Wenson Hsieh
Modified: 2021-01-02 16:56 PST (History)
8 users (show)

See Also:


Attachments
WIP (2.74 KB, patch)
2020-12-23 16:30 PST, Wenson Hsieh
no flags Details | Formatted Diff | Diff
Patch (2.45 KB, patch)
2020-12-27 11:20 PST, Wenson Hsieh
sam: review-
Details | Formatted Diff | Diff
Patch (2.37 KB, patch)
2020-12-27 13:22 PST, Wenson Hsieh
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Wenson Hsieh 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)
Comment 1 Wenson Hsieh 2020-12-23 16:30:21 PST
Created attachment 416730 [details]
WIP
Comment 2 Wenson Hsieh 2020-12-27 11:20:08 PST
Created attachment 416823 [details]
Patch
Comment 3 Sam Weinig 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.
Comment 4 Wenson Hsieh 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.
Comment 5 Wenson Hsieh 2020-12-27 13:22:20 PST
Created attachment 416825 [details]
Patch
Comment 6 Wenson Hsieh 2020-12-27 22:41:30 PST
Comment on attachment 416825 [details]
Patch

Thanks for the review!
Comment 7 EWS 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].
Comment 8 Radar WebKit Bug Importer 2020-12-27 22:46:18 PST
<rdar://problem/72697931>
Comment 9 Darin Adler 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.