Introduce til/latch.h, til/mutex.h and til/throttled_func.h #10403

lhecker · 2021-06-10T21:35:17Z

This commit introduce three new til features:

"til/latch.h": A std::latch clone, until we're on C++20.
"til/mutex.h": A safe mutex wrapper, which only allows you access to the protected data after locking it. No more forgetting to lock mutexes!
"til/throttled_func.h": Function invocation throttling used to be available as the ThrottledFunc class already. But this class is vastly more efficient and doesn't rely on any WinRT types.

This PR also adds a til::ends_with string helper which is til::starts_with counterpart.

Validation Steps Performed

Scrollbar throttling still works as it used to ✔️
No performance regressions when printing big.txt ✔️

Closes #10393

src/cascadia/TerminalControl/ThrottledFunc.h

DHowett

make sure you're adding tests for the til things you're adding 😄 wherever easily possible of course

src/inc/til/strings.h

src/inc/til/throttled_func.h

lhecker · 2021-06-10T23:30:15Z

I've rebased this PR to resolve:

The merge conflict with the main branch
The race condition I found in my ThrottledFunc implementation (not to be confused with til::throttled_func^^)

No further changes were made.

KalleOlaviNiemitalo · 2021-06-11T04:48:32Z

src/inc/til/mutex.h

+        }
+
+    private:
+        mutable T _data{};


I didn't expect _data to be mutable. Now there's no difference between const and non-const shared_mutex because all data members are mutable and all member functions are const.

That's exactly right! This is because this mutex class encapsulates not just the data, but also const-correctness! In Rust this concept is called "interior mutability".

The mutex ensures const-correctness, by enforcing that only a single writer at a time can acquire a non-const reference to the inner (protected) data using a call to .lock(). If no routine has currently .lock()ed the mutex, multiple const references to the inner data can be created, by calling .lock_shared(). That's why _data is mutable: The mutex itself controls const-correctness.

BTW I also tested today whether there's any difference in the generated assembly between using mutable and not and there doesn't seem to be any.

But this isn't really surprising anyways: As usual C++ continues to not have any rules forbidding aliasing non-const references, which sucks 🍑. Meaning: Every time (if on MSVC without strict aliasing support) or almost every time (with strict aliasing) you dereference a value in C++ you fetch it's value from memory, because the compiler can't prove you didn't change it as a side-effect somewhere else. In C++ every pointer is basically something of a 50%-volatile pointer. So yeah... mutable doesn't make it worse, because it's already suboptimal by default lol.

Oh, I wasn't thinking about aliasing and code generation. Rather, I was thinking about having a til::shared_mutex mutex data member in some type X. Even const member functions of X can then exclusively lock and mutate the data within X::mutex. It seems to me that, if the const member functions of X need to do that, then having X::mutex itself be mutable would make the code easier to understand. I am not familiar with Rust, though.

Edit: This is a long answer, the short one is below
My first comment explains the reasoning why this mutex works even if in const member functions of X.
The gist of it was:
Imagine a programmer accidentally uses .lock_shared() in a non-const method and modify the data protected by the mutex! All hell would break loose, because you just created a race condition.
By tying const-ness to the way you locked (exclusive/shared) you can get much better safety instead.
When you use .lock_shared() with this mutex you can never get a non-const reference and accidentally modify the data.

But let's talk about your concern:

If you use a class instance across threads in, the sharing must occur using constant references or a std::shared_ptr<const T>. Why? Well if your class has any methods that aren't thread-safe, all such methods must be marked const, otherwise you risk a race condition. To ensure this safety, cross-thread sharing must always use const-ness.... technically. I know the real world works differently, but that's really just an unfortunate of C++ age, isn't it?

One solution would be to make the til::shared_mutex member of the class itself mutable, allowing you to call .lock(), like you said. But this would run counter to the very idea this class represents: It encapsulates cross-thread shared state and the const-ness of its data, by the way you locked it, and not by whether the caller is const or not. In fact it doesn't matter whether the caller is const, because the caller should always be const when this class is used (due to the prior point). But yeah, I could add the mutable keyword anywhere this class is used instead for sure, but it would certainly be against the concept and ideals this class represents. Especially since many probably agree that the mutable keyword is almost as bad as const_cast itself. I mean it does the same thing after all. Personally I'd rather have this anti-pattern encapsulated just once, which is why I did it this way...

Damn.... Now that I wrote all this I realize a much much shorter answer:
You cannot have any mutable references to a class instance once it's potentially shared across threads.
So why bother making this mutex work with the old C++ concept of "the caller's constness determines the callee's constness", when the former must always be const anyways?

lhecker · 2021-06-11T23:04:41Z

@DHowett Unit tests added! ✔️

src/cascadia/TerminalControl/ThrottledFunc.h

DHowett

Just a couple questions!

DHowett · 2021-06-16T22:21:21Z

src/cascadia/WinRTUtils/inc/ThrottledFunc.h

+    winrt::Windows::UI::Core::CoreDispatcher _dispatcher;
+    function _func;
+    wil::unique_threadpool_timer _timer;
+    til::details::throttled_func_storage<Args...> _storage;


this is technically a violation of til's private space by an external library. Is there a benefit to replacing the currently implementation of ThrottledFunc with this new one? Or should we just leave the current one alone (move it to WinRTUtils)?

I see that it cleverly splits the implementation into leading/trailing edge, and I do appreciate that it is cleaner.

I added some improvements to til::details::throttled_func_storage, which weren't in ThrottledFuncStorage. As I consider ThrottledFunc a bit of a hack, I also considered it fine to use the private namespace.
I can definitely add ThrottledFuncStorage back!

DHowett · 2021-06-16T22:23:31Z

src/inc/til/throttled_func.h

+                throw std::invalid_argument("non-positive delay specified");
+            }
+
+            memcpy(&_delay, &d, sizeof(d));


This seems like an overly clever trick. Does the compiler not generate a sufficient copy when you use _delay = d?

(Tricks like this seem like a micro-optimization, and micro-optimizations aren't bad but they do deserve some scrutiny and discussion about their specific purpose 😄)

Actually _delay is a FILETIME and d is a int64_t.
I considered making _delay an int64_t as well, but this would require a reinterpret_cast between two unrelated types, which is technically undefined behavior and could break with any VS upgrade.
C++20 introduced std::bit_cast which would allow me to write _delay = std::bit_cast<FILETIME>(-delay.count()), but we don't have C++20 enabled yet (std::bit_cast is basically memcpy, but from the compiler and constexpr).

DHowett · 2021-06-16T22:29:40Z

src/inc/til/throttled_func.h

+        // Makes sure that the currently pending timer is executed
+        // as soon as possible and in that case waits for its completion.
+        // You can use this function in your destructor to ensure that any
+        // pending callback invocation is completed as soon as possible.


In the original implementation:

If the callback was mid-firing while the caller gave up on it, it would keep itself alive and finish.

If the callback had been scheduled, but not called, while the caller gave up on it, it would not fire.

If the callback had not been scheduled, and the caller gave up on it, it would not fire.

This seems like an implementation that's different in an important way:
In cases 1 and 2, the callback will always complete. You must wait for it during destruction, and the throttled_func must remain alive for the callback's complete lifetime (scheduling + execution)...?

Maybe I'm misunderstanding you, but the difference is:
When the new throttled_func is dropped/destroyed it'll cancel any timers automatically. If a callback is currently being executed in a background thread, the throttled_func will synchronously block in the destructor until the callback finished. It would be quite bad if you hold a mutex, while you destroy a throttled_func...

DHowett · 2021-06-16T22:30:23Z

src/inc/til/throttled_func.h

+
+        FILETIME _delay;
+        function _func;
+        wil::unique_threadpool_timer _timer;


Does the destruction of a unique_threadpool_timer deschedule it?

Yep! The destructor of wil::unique_threadpool_timer calls WaitForThreadpoolTimerCallbacks(_timer, true) and thus cancels any pending timers.

DHowett · 2021-06-16T22:31:49Z

src/inc/til/throttled_func.h

+            mutable std::shared_mutex _lock;
+            std::optional<std::tuple<Args...>> _pendingRunArgs;


Did you consider making this use til::mutex?

I did, but I wanted to keep both header files independent of each other and keep their template complexity minimal. Would you suggest that I use til::shared_mutex here?

src/til/ut_til/throttling_func.cpp

DHowett · 2021-06-16T22:34:46Z

src/inc/til/string.h

+    constexpr bool ends_with(const std::basic_string_view<T, Traits> str, const std::basic_string_view<T, Traits> prefix) noexcept
+    {
+#ifdef __cpp_lib_ends_ends_with
+#error This code can be replaced in C++20, which natively supports .ends_with().


Is it more meaningful to smack developers who try to switch to C++20, or simply call ends_with ourselves? 😉

SMACK 'EM! 👊

Jokes aside: Maybe a #warning? The idea behind the #error is that if anyone ever upgrades to C++20, they should be able to remove the #error line, if they believe that's appropriate.
What's your opinion about this?

src/cascadia/WinRTUtils/inc/ThrottledFunc.h

src/inc/til/latch.h

ghost · 2021-06-22T19:49:42Z

Hello @DHowett!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.

This commit introduce three new `til` features: * "til/latch.h": A std::latch clone, until we're on C++20. * "til/mutex.h": A safe mutex wrapper, which only allows you access to the protected data after locking it. No more forgetting to lock mutexes! * "til/throttled_func.h": Function invocation throttling used to be available as the `ThrottledFunc` class already. But this class is vastly more efficient and doesn't rely on any WinRT types. This PR also adds a `til::ends_with` string helper which is `til::starts_with` counterpart. * Scrollbar throttling still works as it used to ✔️ * No performance regressions when printing big.txt ✔️ Closes #10393 (cherry picked from commit 0d9a357)

ghost · 2021-07-14T17:04:11Z

🎉Windows Terminal Preview v1.10.1933.0 has been released which incorporates this pull request.:tada:

Handy links:

lhecker requested a review from DHowett June 10, 2021 21:35

ghost added Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. Product-Terminal The new Windows Terminal. labels Jun 10, 2021

This comment has been minimized.

Sign in to view

lhecker force-pushed the dev/lhecker/til-goodies branch from 1309b72 to 211c67b Compare June 10, 2021 21:43

lhecker commented Jun 10, 2021

View reviewed changes

src/cascadia/TerminalControl/ThrottledFunc.h Outdated Show resolved Hide resolved

DHowett reviewed Jun 10, 2021

View reviewed changes

src/inc/til/strings.h Outdated Show resolved Hide resolved

skyline75489 reviewed Jun 10, 2021

View reviewed changes

src/inc/til/throttled_func.h Outdated Show resolved Hide resolved

Introduce til/mutex.h and til/throttled_func.h

9ff7fc9

lhecker force-pushed the dev/lhecker/til-goodies branch from 211c67b to 9ff7fc9 Compare June 10, 2021 23:27

lhecker changed the title ~~Introduce til/mutex.h, strings.h and throttled_func.h~~ Introduce til/mutex.h and til/throttled_func.h Jun 10, 2021

lhecker added 3 commits June 11, 2021 01:41

Fix AuditMode errors

9325834

Remove remaining bits of WinRT from til::throttled_func

0615204

Make til::throttled_func member naming more consistent

738316d

This comment has been minimized.

Sign in to view

Fix x86 compilation issue

582af8b

KalleOlaviNiemitalo reviewed Jun 11, 2021

View reviewed changes

Add unit tests

a80d2a9

lhecker added 2 commits June 12, 2021 01:05

Fix formatting

28bc69e

Fix spelling check

cc15525

microsoft deleted a comment from github-actions bot Jun 11, 2021

lhecker added 2 commits June 14, 2021 14:20

Improve documentation

5bc68da

Fix spelling check

fa05441

microsoft deleted a comment from github-actions bot Jun 14, 2021

miniksa reviewed Jun 14, 2021

View reviewed changes

src/cascadia/TerminalControl/ThrottledFunc.h Outdated Show resolved Hide resolved

lhecker added 2 commits June 14, 2021 19:05

ThrottledFunc shall henceforth be found in WinRTUtils

cb9895e

Remove leftovers from previous co_await experiment

c8693b7

microsoft deleted a comment from github-actions bot Jun 14, 2021

miniksa approved these changes Jun 15, 2021

View reviewed changes

DHowett requested changes Jun 16, 2021

View reviewed changes

ghost added Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something and removed Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something labels Jun 16, 2021

Added til::latch, addressed reviewer comments

d969db0

lhecker changed the title ~~Introduce til/mutex.h and til/throttled_func.h~~ Introduce til/latch.h, til/mutex.h and til/throttled_func.h Jun 17, 2021

DHowett reviewed Jun 17, 2021

View reviewed changes

src/cascadia/WinRTUtils/inc/ThrottledFunc.h Outdated Show resolved Hide resolved

Fix unsafe use of DisassociateCurrentThreadFromCallback

3c35e91

microsoft deleted a comment from github-actions bot Jun 17, 2021

Fix spelling check

f081585

microsoft deleted a comment from github-actions bot Jun 18, 2021

lhecker added 2 commits June 18, 2021 21:56

Simplify ThrottledFunc

a0898a3

Remove latch.cpp

59c33b4

DHowett approved these changes Jun 22, 2021

View reviewed changes

src/inc/til/latch.h Outdated Show resolved Hide resolved

Address reviewer comments

80029e8

DHowett approved these changes Jun 22, 2021

View reviewed changes

DHowett added the AutoMerge Marked for automatic merge by the bot when requirements are met label Jun 22, 2021

lhecker added AutoMerge Marked for automatic merge by the bot when requirements are met and removed AutoMerge Marked for automatic merge by the bot when requirements are met labels Jun 22, 2021

ghost merged commit 0d9a357 into main Jun 22, 2021

ghost deleted the dev/lhecker/til-goodies branch June 22, 2021 20:16

ghost mentioned this pull request Jul 14, 2021

Should we have a ThrottledFunc that works without a dispatcher? #10393

Closed

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce til/latch.h, til/mutex.h and til/throttled_func.h #10403

Introduce til/latch.h, til/mutex.h and til/throttled_func.h #10403

lhecker commented Jun 10, 2021 •

edited by DHowett

Loading

This comment has been minimized.

DHowett left a comment

lhecker commented Jun 10, 2021

This comment has been minimized.

KalleOlaviNiemitalo Jun 11, 2021

lhecker Jun 11, 2021

lhecker Jun 12, 2021 •

edited

Loading

KalleOlaviNiemitalo Jun 14, 2021

lhecker Jun 14, 2021 •

edited

Loading

lhecker Jun 14, 2021 •

edited

Loading

lhecker commented Jun 11, 2021

DHowett left a comment

DHowett Jun 16, 2021

DHowett Jun 16, 2021

lhecker Jun 16, 2021

DHowett Jun 16, 2021

DHowett Jun 16, 2021

lhecker Jun 17, 2021

DHowett Jun 16, 2021

lhecker Jun 17, 2021

DHowett Jun 16, 2021

lhecker Jun 17, 2021

DHowett Jun 16, 2021

lhecker Jun 17, 2021

DHowett Jun 16, 2021

lhecker Jun 17, 2021 •

edited

Loading

ghost commented Jun 22, 2021

ghost commented Jul 14, 2021

		mutable std::shared_mutex _lock;
		std::optional<std::tuple<Args...>> _pendingRunArgs;

Introduce til/latch.h, til/mutex.h and til/throttled_func.h #10403

Introduce til/latch.h, til/mutex.h and til/throttled_func.h #10403

Conversation

lhecker commented Jun 10, 2021 • edited by DHowett Loading

Validation Steps Performed

This comment has been minimized.

DHowett left a comment

Choose a reason for hiding this comment

lhecker commented Jun 10, 2021

This comment has been minimized.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lhecker Jun 12, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lhecker Jun 14, 2021 • edited Loading

Choose a reason for hiding this comment

lhecker Jun 14, 2021 • edited Loading

Choose a reason for hiding this comment

lhecker commented Jun 11, 2021

DHowett left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lhecker Jun 17, 2021 • edited Loading

Choose a reason for hiding this comment

ghost commented Jun 22, 2021

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

ghost commented Jul 14, 2021

lhecker commented Jun 10, 2021 •

edited by DHowett

Loading

lhecker Jun 12, 2021 •

edited

Loading

lhecker Jun 14, 2021 •

edited

Loading

lhecker Jun 14, 2021 •

edited

Loading

lhecker Jun 17, 2021 •

edited

Loading

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (`@msftbot`) and give me an instruction to get started! Learn more here.