Call UpdateJumplist only if the settings changed #13692

lhecker · 2022-08-05T21:06:22Z

This commit stores a hash of the settings.json file in ApplicationState
with which we can detect whether the settings contents actually changed.
Since I only use a small 64-bit hash as opposed to SHA2 for instance,
I'm taking the last write time of the file into account as well.
This allows us to skip calling UpdateJumplist at least the majority of app
launches which hopefully improves launch performance on devices with slower IO.

Part of #5907.

Validation Steps Performed

Delete some profiles (see above), save settings, tasks are gone ✅
FYI For some (...) inexplicable reason, shell task lists are preserved forever
even if msix applications are uninstalled, etc. So to test whether tasks are
properly written on first app launch we have to delete some profiles/tasks
first, otherwise we can't see whether they're actually written later.
Now exit Windows Terminal, delete settings.json and relaunch
All tasks are back ✅
With a debugger, ensure that CascadiaSettings::WriteSettingsToDisk
generates the same hash that LoadAll reads. ✅

zadjii-msft

What happens if the user restarts their PC, but the settings file hasn't changed? Does the shell remember their jumplist from last time?

zadjii-msft · 2022-08-08T11:52:49Z

Other thought from teams:

on a first ever launch, this will still generate the jumplist as we're creating the window, right? Like, we still want to defer that until after StartupState::Initialized

DHowett · 2022-08-08T21:56:23Z

What happens if the user restarts their PC, but the settings file hasn't changed? Does the shell remember their jumplist from last time?

Jumplists are durable across restarts, for sure.

lhecker · 2022-08-10T23:17:53Z

What happens if the user restarts their PC, but the settings file hasn't changed? Does the shell remember their jumplist from last time?

I tested it and it works fine.

on a first ever launch, this will still generate the jumplist as we're creating the window, right? Like, we still want to defer that until after StartupState::Initialized

As mentioned "offline", I'm not convinced that this will bring noticeable improvements. One reason being that an AV like Windows Defender will have a much greater impact on us on first launch than anything we do ourselves.

But it is a convincing argument! Unfortunately I got a bit stumped now on how to best implement this...
On first launch CascadiaSettings::LoadAll will write the initial settings.json to disk which will trigger AppLogic::_ReloadSettings. This happens before the UI is fully interactive, since settings are required to be present at launch and the reload is scheduled via the Dispatcher(). Even at low priority this reload happens before full interactivity is reached.
Should we build some sort of system to delay _ReloadSettings during launch? I wasn't sure it would be considered worth the effort... it would require us to block a background thread with a semaphore or similar.

zadjii-msft · 2022-08-11T11:33:21Z

Okay idea:

In AppLogic::_ReloadSettings, we check if we're in _startupState >= Initialized.

If we are, cool, we're past the first paint. Kick off the bg thread to update the jumplist, it's fine.
If we aren't, we're still in the initial launch. Register a handler for page.Initialized, and then we can revoke that handler once it fires. Once we get to Initialized, the window is fully ready to go.

Thoughts?

lhecker · 2022-08-15T13:33:05Z

I've come up with an easier approach which doesn't need to check _startupState.
Since the PR is a bit different now, I've rebased it on main.

zadjii-msft

Sure let's give it a whirl

zadjii-msft · 2022-08-24T18:03:05Z

src/cascadia/TerminalSettingsModel/FileUtils.cpp

@@ -179,18 +179,23 @@ namespace winrt::Microsoft::Terminal::Settings::Model
                buffer.erase(0, Utf8Bom.size());
            }

+            if (lastWriteTime)
+            {
+                THROW_IF_WIN32_BOOL_FALSE(GetFileTime(file.get(), nullptr, nullptr, lastWriteTime));


THROW? or LOG? Can this reasonably fail?

don't think it's much worth worrying about in this case.. hopefully

Oh right I forgot to respond that I think throwing is the right thing to do here. A caller should be able to trust us that we either successfully fill all out parameters or return a failure (exception).

But does this leave the opportunity for an unguarded exception that takes down the app? "Caller" is one thing, but "Terminal the app and its reliability" is another

ReadFile in the same function throws exceptions as well.

DHowett

I'm cool with trying this in 1.16 preview

DHowett · 2022-08-31T21:04:21Z

src/cascadia/TerminalSettingsModel/FileUtils.cpp

@@ -179,18 +179,23 @@ namespace winrt::Microsoft::Terminal::Settings::Model
                buffer.erase(0, Utf8Bom.size());
            }

+            if (lastWriteTime)
+            {
+                THROW_IF_WIN32_BOOL_FALSE(GetFileTime(file.get(), nullptr, nullptr, lastWriteTime));


don't think it's much worth worrying about in this case.. hopefully

ghost · 2022-09-13T17:03:52Z

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

Handy links:

zadjii-msft reviewed Aug 8, 2022

View reviewed changes

lhecker marked this pull request as ready for review August 8, 2022 20:42

lhecker force-pushed the dev/lhecker/5907-jumplist-caching branch from e279408 to b784500 Compare August 15, 2022 13:33

This comment was marked as outdated.

Sign in to view

Call UpdateJumplist only if the settings changed

ccf550b

lhecker force-pushed the dev/lhecker/5907-jumplist-caching branch from b784500 to ccf550b Compare August 15, 2022 13:35

lhecker changed the title ~~[Proposal] Call UpdateJumplist only if the settings changed~~ Call UpdateJumplist only if the settings changed Aug 15, 2022

lhecker added Area-Performance Performance-related issue Product-Terminal The new Windows Terminal. Issue-Task It's a feature request, but it doesn't really need a major design. labels Aug 18, 2022

zadjii-msft approved these changes Aug 24, 2022

View reviewed changes

zadjii-msft added the Needs-Second It's a PR that needs another sign-off label Aug 25, 2022

ghost requested review from PankajBhojwani, carlos-zamora and DHowett August 25, 2022 15:50

DHowett approved these changes Aug 31, 2022

View reviewed changes

DHowett added the AutoMerge Marked for automatic merge by the bot when requirements are met label Aug 31, 2022

DHowett merged commit a440e15 into main Aug 31, 2022

DHowett deleted the dev/lhecker/5907-jumplist-caching branch August 31, 2022 21:18

lhecker mentioned this pull request Nov 10, 2022

WT should start up fast: profile the startup path and trim anything that takes a while #5907

Open

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Call UpdateJumplist only if the settings changed #13692

Call UpdateJumplist only if the settings changed #13692

lhecker commented Aug 5, 2022 •

edited

Loading

zadjii-msft left a comment

zadjii-msft commented Aug 8, 2022

DHowett commented Aug 8, 2022

lhecker commented Aug 10, 2022 •

edited

Loading

zadjii-msft commented Aug 11, 2022

lhecker commented Aug 15, 2022 •

edited

Loading

This comment was marked as outdated.

zadjii-msft left a comment

zadjii-msft Aug 24, 2022

DHowett Aug 31, 2022

lhecker Aug 31, 2022

DHowett Aug 31, 2022

lhecker Aug 31, 2022

DHowett left a comment

DHowett Aug 31, 2022

ghost commented Sep 13, 2022

Call UpdateJumplist only if the settings changed #13692

Call UpdateJumplist only if the settings changed #13692

Conversation

lhecker commented Aug 5, 2022 • edited Loading

Validation Steps Performed

zadjii-msft left a comment

Choose a reason for hiding this comment

zadjii-msft commented Aug 8, 2022

DHowett commented Aug 8, 2022

lhecker commented Aug 10, 2022 • edited Loading

zadjii-msft commented Aug 11, 2022

lhecker commented Aug 15, 2022 • edited Loading

This comment was marked as outdated.

zadjii-msft left a comment

Choose a reason for hiding this comment

zadjii-msft Aug 24, 2022

Choose a reason for hiding this comment

DHowett Aug 31, 2022

Choose a reason for hiding this comment

lhecker Aug 31, 2022

Choose a reason for hiding this comment

DHowett Aug 31, 2022

Choose a reason for hiding this comment

lhecker Aug 31, 2022

Choose a reason for hiding this comment

DHowett left a comment

Choose a reason for hiding this comment

DHowett Aug 31, 2022

Choose a reason for hiding this comment

ghost commented Sep 13, 2022

lhecker commented Aug 5, 2022 •

edited

Loading

lhecker commented Aug 10, 2022 •

edited

Loading

lhecker commented Aug 15, 2022 •

edited

Loading