Skip to content

Commit

Permalink
Resolve circular reference in ThrottledFunc (#9729)
Browse files Browse the repository at this point in the history
## Summary of the Pull Request

ThrottledFunc previously created a DispatcherTimer whose Tick callback holds a strong reference to the DispatcherTimer itself.
This causes a reference cycle, inadvertently leaking timer instances.

## PR Checklist

* [x] Closes #7710
* [x] I work here

## Detailed Description of the Pull Request / Additional comments

I've initially wanted to remove the `ThrottledFunc<>` optimization, but it turns out that this causes a 3% slowdown. That's definitely not a lot, but enough that we can just keep the optimization for the time being.
I've moved the implementation from the .cpp file into the header regardless since the two implementations are extremely similar and it's easier that way to keep them in line.

## Validation Steps Performed

I've ensured that the scrollbar still updates its length when I add new lines to a newly created tab.
  • Loading branch information
lhecker authored Apr 6, 2021
1 parent ebd07d7 commit faf372f
Show file tree
Hide file tree
Showing 3 changed files with 55 additions and 79 deletions.
1 change: 0 additions & 1 deletion src/cascadia/TerminalControl/TerminalControlLib.vcxproj
Original file line number Diff line number Diff line change
Expand Up @@ -67,7 +67,6 @@
<ClCompile Include="TermControl.cpp">
<DependentUpon>TermControl.xaml</DependentUpon>
</ClCompile>
<ClCompile Include="ThrottledFunc.cpp" />
<ClCompile Include="TSFInputControl.cpp">
<DependentUpon>TSFInputControl.xaml</DependentUpon>
</ClCompile>
Expand Down
54 changes: 0 additions & 54 deletions src/cascadia/TerminalControl/ThrottledFunc.cpp

This file was deleted.

79 changes: 55 additions & 24 deletions src/cascadia/TerminalControl/ThrottledFunc.h
Original file line number Diff line number Diff line change
Expand Up @@ -56,27 +56,7 @@ class ThrottledFunc : public std::enable_shared_from_this<ThrottledFunc<Args...>
}
}

_dispatcher.RunAsync(CoreDispatcherPriority::Low, [weakThis = this->weak_from_this()]() {
if (auto self{ weakThis.lock() })
{
DispatcherTimer timer;
timer.Interval(self->_delay);
timer.Tick([=](auto&&...) {
if (auto self{ weakThis.lock() })
{
timer.Stop();

std::optional<std::tuple<Args...>> args;
{
std::lock_guard guard{ self->_lock };
self->_pendingRunArgs.swap(args);
}
std::apply(self->_func, args.value());
}
});
timer.Start();
}
});
_Fire(_delay, _dispatcher, this->weak_from_this());
}

// Method Description:
Expand Down Expand Up @@ -110,12 +90,29 @@ class ThrottledFunc : public std::enable_shared_from_this<ThrottledFunc<Args...>
}

private:
static winrt::fire_and_forget _Fire(winrt::Windows::Foundation::TimeSpan delay, winrt::Windows::UI::Core::CoreDispatcher dispatcher, std::weak_ptr<ThrottledFunc> weakThis)
{
co_await winrt::resume_after(delay);
co_await winrt::resume_foreground(dispatcher);

if (auto self{ weakThis.lock() })
{
std::optional<std::tuple<Args...>> args;
{
std::lock_guard guard{ self->_lock };
self->_pendingRunArgs.swap(args);
}

std::apply(self->_func, args.value());
}
}

Func _func;
winrt::Windows::Foundation::TimeSpan _delay;
winrt::Windows::UI::Core::CoreDispatcher _dispatcher;

std::optional<std::tuple<Args...>> _pendingRunArgs;
std::mutex _lock;
std::optional<std::tuple<Args...>> _pendingRunArgs;
};

// Class Description:
Expand All @@ -129,11 +126,45 @@ class ThrottledFunc<> : public std::enable_shared_from_this<ThrottledFunc<>>
public:
using Func = std::function<void()>;

ThrottledFunc(Func func, winrt::Windows::Foundation::TimeSpan delay, winrt::Windows::UI::Core::CoreDispatcher dispatcher);
ThrottledFunc(Func func, winrt::Windows::Foundation::TimeSpan delay, winrt::Windows::UI::Core::CoreDispatcher dispatcher) :
_func{ func },
_delay{ delay },
_dispatcher{ dispatcher }
{
}

void Run();
// Method Description:
// - Runs the function later, except if `Run` is called again before
// with a new argument, in which case the request will be ignored.
// - For more information, read the class' documentation.
// - This method is always thread-safe. It can be called multiple times on
// different threads.
// Arguments:
// - <none>
// Return Value:
// - <none>
template<typename... MakeArgs>
void Run(MakeArgs&&... args)
{
if (!_isRunPending.test_and_set(std::memory_order_relaxed))
{
_Fire(_delay, _dispatcher, this->weak_from_this());
}
}

private:
static winrt::fire_and_forget _Fire(winrt::Windows::Foundation::TimeSpan delay, winrt::Windows::UI::Core::CoreDispatcher dispatcher, std::weak_ptr<ThrottledFunc> weakThis)
{
co_await winrt::resume_after(delay);
co_await winrt::resume_foreground(dispatcher);

if (auto self{ weakThis.lock() })
{
self->_isRunPending.clear(std::memory_order_relaxed);
self->_func();
}
}

Func _func;
winrt::Windows::Foundation::TimeSpan _delay;
winrt::Windows::UI::Core::CoreDispatcher _dispatcher;
Expand Down

0 comments on commit faf372f

Please sign in to comment.