Resolve circular reference in ThrottledFunc #9729

lhecker · 2021-04-06T16:09:16Z

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

Closes Terminal gets extremely sluggish after a day of use #7710
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.

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.

lhecker · 2021-04-06T16:14:55Z

src/cascadia/TerminalControl/ThrottledFunc.h

@@ -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)


FYI: I've tried using a lambda instead of a static function, but the code turned out to be about as ugly (due to the need to explicitly capture every member variable). I've chosen the static function as it at least ensures that the this pointer isn't accidentally used.

Sounds great to me. Good point on avoiding accidental usage.

So cool that we got to move it to the header!

miniksa · 2021-04-06T16:15:43Z

References

This PR potentially solves the issues seen in #7710.

Pretty sure it has to say "Closes " for GitHub to automatically pick up the connection and tie them together.

lhecker · 2021-04-06T16:19:24Z

@miniksa I wasn't sure whether I should write that it closes that issue since we actually don't know what the cause for the severe slowdown is for the affected people. While we both can reproduce the timer leak locally, neither of us could reproduce a slowdown that causes such severe issues though, right?
But you gave me the idea to try and replicate it by slowly dumping a lot of text in a lot of tabs. I think the "slow" part is what amplifies the issue.

lhecker · 2021-04-06T16:36:12Z

I was able to replicate the issue by running the following "slow cat" in 30 tabs simultaneously:

cat big.txt | while read l; echo $l; sleep 0.05; end

After about 10min the current store version started feeling sluggish, whereas this one didn't.
--> This PR should fix #7710 entirely.

miniksa

Let me pull this branch and do the heap trace to see if the leak is gone.

miniksa · 2021-04-06T16:46:39Z

src/cascadia/TerminalControl/ThrottledFunc.h

@@ -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)


Sounds great to me. Good point on avoiding accidental usage.

miniksa · 2021-04-06T16:54:14Z

@miniksa I wasn't sure whether I should write that it closes that issue since we actually don't know what the cause for the severe slowdown is for the affected people. While we both can reproduce the timer leak locally, neither of us could reproduce a slowdown that causes such severe issues though, right?
But you gave me the idea to try and replicate it by slowly dumping a lot of text in a lot of tabs. I think the "slow" part is what amplifies the issue.

You're right that we're not 100% sure this will fix the issue for folks until they try it out, but we have a lot of reasons to be confident in it. It's a slow leak over time. It's on the UI dispatcher thread which makes everything slow. This throttler was introduced in June and the bug opened in September so by time proximity and the release schedule, it correlates. And the root cause and resolution make sense for the experience people are having.

Also... we can always reopen the issue or duplicate it into a new one if we close it and it turns out to not work. So yeah. Just go for "Closes" when you're relatively confident.

DHowett · 2021-04-06T19:03:22Z

YES LEONARD'S FIRST PULL REQUEST

DHowett

EX CELLENT

DHowett · 2021-04-06T19:06:16Z

@msftbot merge this in like 2ish minutes

ghost · 2021-04-06T19:06:18Z

Hello @DHowett!

I think you told me that you want to delay the approval for a certain amount of time, but I am not confident that I have understood you correctly.

Please try rephrasing your instruction to me.

DHowett · 2021-04-06T19:06:26Z

@msftbot merge this in 1 minute

ghost · 2021-04-06T19:06:29Z

Hello @DHowett!

Because you've given me some instructions on how to help merge this pull request, I'll be modifying my merge approach. Here's how I understand your requirements for merging this pull request:

I won't merge this pull request until after the UTC date Tue, 06 Apr 2021 19:07:27 GMT, which is in 1 minute

If this doesn't seem right to you, you can tell me to cancel these instructions and use the auto-merge policy that has been configured for this repository. Try telling me "forget everything I just told you".

## 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. (cherry picked from commit faf372f)

ghost · 2021-04-14T17:03:21Z

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

Handy links:

ghost · 2021-04-14T17:06:42Z

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

Handy links:

Resolve circular reference in ThrottledFunc

821aac6

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.

lhecker requested review from miniksa and zadjii-msft April 6, 2021 16:09

lhecker commented Apr 6, 2021

View reviewed changes

ghost added Area-Performance Performance-related issue Issue-Bug It either shouldn't be doing this or needs an investigation. Priority-2 A description (P2) Product-Terminal The new Windows Terminal. labels Apr 6, 2021

miniksa reviewed Apr 6, 2021

View reviewed changes

miniksa approved these changes Apr 6, 2021

View reviewed changes

DHowett approved these changes Apr 6, 2021

View reviewed changes

ghost added the AutoMerge Marked for automatic merge by the bot when requirements are met label Apr 6, 2021

ghost merged commit faf372f into main Apr 6, 2021

ghost deleted the dev/lhecker/issue-7710-throttled-func branch April 6, 2021 19:07

DHowett added zPreview-Service-Queued-1.13 A floating label that tracks the current Preview version for servicing purposes. and removed zPreview-Service-Queued-1.13 A floating label that tracks the current Preview version for servicing purposes. labels Apr 6, 2021

ghost mentioned this pull request Apr 14, 2021

Terminal gets extremely sluggish after a day of use #7710

Closed

zadjii-msft mentioned this pull request Apr 23, 2021

Terminal crashing when last tab is closed with exit, 2021 edition #9910

Closed

This pull request was closed.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Resolve circular reference in ThrottledFunc #9729

Resolve circular reference in ThrottledFunc #9729

lhecker commented Apr 6, 2021 •

edited

Loading

lhecker Apr 6, 2021

miniksa Apr 6, 2021

DHowett Apr 6, 2021

miniksa commented Apr 6, 2021

References

lhecker commented Apr 6, 2021

lhecker commented Apr 6, 2021

miniksa left a comment

miniksa Apr 6, 2021

miniksa commented Apr 6, 2021

DHowett commented Apr 6, 2021

DHowett left a comment

DHowett commented Apr 6, 2021

ghost commented Apr 6, 2021

DHowett commented Apr 6, 2021

ghost commented Apr 6, 2021

ghost commented Apr 14, 2021

ghost commented Apr 14, 2021

Resolve circular reference in ThrottledFunc #9729

Resolve circular reference in ThrottledFunc #9729

Conversation

lhecker commented Apr 6, 2021 • edited Loading

Summary of the Pull Request

PR Checklist

Detailed Description of the Pull Request / Additional comments

Validation Steps Performed

lhecker Apr 6, 2021

Choose a reason for hiding this comment

miniksa Apr 6, 2021

Choose a reason for hiding this comment

DHowett Apr 6, 2021

Choose a reason for hiding this comment

miniksa commented Apr 6, 2021

References

lhecker commented Apr 6, 2021

lhecker commented Apr 6, 2021

miniksa left a comment

Choose a reason for hiding this comment

miniksa Apr 6, 2021

Choose a reason for hiding this comment

miniksa commented Apr 6, 2021

DHowett commented Apr 6, 2021

DHowett left a comment

Choose a reason for hiding this comment

DHowett commented Apr 6, 2021

ghost commented Apr 6, 2021

DHowett commented Apr 6, 2021

ghost commented Apr 6, 2021

ghost commented Apr 14, 2021

ghost commented Apr 14, 2021

lhecker commented Apr 6, 2021 •

edited

Loading