Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Move jumplist creation to background thread #7978

Merged
merged 4 commits into from
Oct 23, 2020
Merged

Move jumplist creation to background thread #7978

merged 4 commits into from
Oct 23, 2020

Conversation

PankajBhojwani
Copy link
Contributor

@PankajBhojwani PankajBhojwani commented Oct 20, 2020 •
edited
Loading

Summary of the Pull Request

Move jumplist creation to a background thread, it does not need to be on the main thread

Reference

#7791

PR Checklist

Detailed Description of the Pull Request / Additional comments

We were not using the HRESULT return value of UpdateJumplist anywhere, so I changed its return type to winrt::fire_and_forget instead. Also replaced all RETURN_IF_FAILED calls in the function to THROW_IF_FAILED to accommodate the change in return type.

Validation Steps Performed

Tracing with a release build:
perf

@ghost ghost added Area-Performance Performance-related issue Issue-Task It's a feature request, but it doesn't really need a major design. Product-Terminal The new Windows Terminal. labels Oct 20, 2020
@PankajBhojwani
Copy link
Contributor Author

@leonMSFT would like your thoughts on this in case there's something I've missed/overlooked!

leonMSFT
leonMSFT approved these changes Oct 20, 2020
View reviewed changes
Copy link
Contributor

@leonMSFT leonMSFT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, thanks! 😄

DHowett
DHowett reviewed Oct 20, 2020
View reviewed changes
Copy link
Member

@DHowett DHowett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wow! Does it actually work? I know the shell APIs can be very picky about how they're called.

Are you getting any logged messages in the output window? If you make a change to your profiles, do they actually get reflected in the jumplist?

@PankajBhojwani
Copy link
Contributor Author

If you make a change to your profiles, do they actually get reflected in the jumplist?

I tested with adding a new profile to my settings file (while the app was running) and yes, the jumplist got updated!

miniksa
miniksa approved these changes Oct 20, 2020
View reviewed changes
Copy link
Member

@miniksa miniksa left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it works fine, I'm good with it.

DHowett
DHowett requested changes Oct 20, 2020
View reviewed changes
src/cascadia/TerminalApp/Jumplist.cpp
@@ -118,8 +118,9 @@ static std::wstring_view _getExePath()
// - settings - The settings object to update the jumplist with.
// Return Value:
// - <none>
HRESULT Jumplist::UpdateJumplist(const CascadiaSettings& settings) noexcept
winrt::fire_and_forget Jumplist::UpdateJumplist(const CascadiaSettings& settings) noexcept
{
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we might want to grab a copy of settings and operate on it inside the coroutine ... otherwise, this reference will 100% be invalid by the time we resume the thread.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed

@ghost ghost added the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Oct 20, 2020
@ghost ghost removed the Needs-Author-Feedback The original author of the issue/PR needs to come back and respond to something label Oct 21, 2020
DHowett
DHowett reviewed Oct 21, 2020
View reviewed changes
src/cascadia/TerminalApp/Jumplist.cpp Outdated
Comment on lines 123 to 127
// make sure to copy the settings _before_ the co_await
const auto settingsCopy = settings.Copy();

co_await winrt::resume_background();

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

whoa boy sorry, hold up

we don't need to copy the settings. I apologize for my confusing wording.

We just need a non-reference by-value capture of settings. A "Copy", in the C++ sense, of the value of settings. When you that with a C++/WinRT type, it increments the reference count and makes sure the reference count stays incremented until your local version is out of scope.

const auto strongSettings = settings;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah. Thank you for the clarification, will remove the call to Copy().

I'm going to leave the ActionAndArgs copy fix in there though - I think that's a bug

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

removed the copy fix to make a new PR for it

@microsoft-github-updates microsoft-github-updates bot changed the base branch from master to main October 22, 2020 00:26
@DHowett DHowett merged commit 4f39e8e into main Oct 23, 2020
@DHowett DHowett deleted the dev/pabhoj/optimize_jumplist branch October 23, 2020 00:17
DHowett pushed a commit that referenced this pull request Oct 27, 2020
@PankajBhojwani @DHowett
Move jumplist creation to background thread (#7978)
fe4f0f3 
Move jumplist creation to a background thread, as it
does not need to be on the main thread

Closes #7791

(cherry picked from commit 4f39e8e)
@ghost
Copy link

ghost commented Nov 11, 2020

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

Handy links:

@ghost ghost mentioned this pull request Nov 11, 2020
Closed
@ghost
Copy link

ghost commented Nov 11, 2020

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

Handy links:

@zadjii-msft zadjii-msft mentioned this pull request Feb 24, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@leonMSFT leonMSFT leonMSFT approved these changes

@zadjii-msft zadjii-msft zadjii-msft approved these changes

@miniksa miniksa miniksa approved these changes

@DHowett DHowett DHowett approved these changes

Assignees

@DHowett DHowett

Labels
Area-Performance Performance-related issue Issue-Task It's a feature request, but it doesn't really need a major design. Product-Terminal The new Windows Terminal.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Optimize the creation of the jumplist entries
5 participants
@PankajBhojwani @DHowett @miniksa @zadjii-msft @leonMSFT