Skip to content

Commit 41d81f3

Browse files
zadjii-msftsadirano
authored andcommitted
Lazy-fetch the settings for extensions (microsoft#38844)
This PR stops us from synchronously initializing the settings page for every extension (including built-in's) on startup. That incurs a small penalty that really adds up the more extensions a user has. Instead, we'll now only initialize the `CommandSettings` object when we first actually need it. From a relatively unscientific test, this saves approximately 10% on the initialization of builtin commands, and for my setup, it trims about 28% off extension initialization (across all built-in's / extensions): branch | Built-in load (ms) | Extension load (ms) | %Δ builtin | %Δ extensions | -- | -- | -- | -- | -- | main | 1455 | 6867.6 | | | this PR | 1309.2 | 4919 | -10.02% | -28.37% Closes microsoft#38321
1 parent fcdc8e9 commit 41d81f3

File tree

5 files changed

+144
-25
lines changed

5 files changed

+144
-25
lines changed

src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/CommandProviderWrapper.cs

Lines changed: 6 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -66,8 +66,10 @@ public CommandProviderWrapper(ICommandProvider provider, TaskScheduler mainThrea
6666
DisplayName = provider.DisplayName;
6767
Icon = new(provider.Icon);
6868
Icon.InitializeProperties();
69+
70+
// Note: explicitly not InitializeProperties()ing the settings here. If
71+
// we do that, then we'd regress GH #38321
6972
Settings = new(provider.Settings, this, _taskScheduler);
70-
Settings.InitializeProperties();
7173

7274
Logger.LogDebug($"Initialized command provider {ProviderId}");
7375
}
@@ -151,9 +153,11 @@ public async Task LoadTopLevelCommands(IServiceProvider serviceProvider, WeakRef
151153
Icon = new(model.Icon);
152154
Icon.InitializeProperties();
153155

156+
// Note: explicitly not InitializeProperties()ing the settings here. If
157+
// we do that, then we'd regress GH #38321
154158
Settings = new(model.Settings, this, _taskScheduler);
155-
Settings.InitializeProperties();
156159

160+
// We do need to explicitly initialize commands though
157161
InitializeCommands(commands, fallbacks, serviceProvider, pageContext);
158162

159163
Logger.LogDebug($"Loaded commands from {DisplayName} ({ProviderId})");
@@ -194,21 +198,6 @@ private void InitializeCommands(ICommandItem[] commands, IFallbackCommandItem[]
194198
}
195199
}
196200

197-
/* This is a View/ExtensionHost piece
198-
* public void AllowSetForeground(bool allow)
199-
{
200-
if (!IsExtension)
201-
{
202-
return;
203-
}
204-
205-
var iextn = extensionWrapper?.GetExtensionObject();
206-
unsafe
207-
{
208-
PInvoke.CoAllowSetForegroundWindow(iextn);
209-
}
210-
}*/
211-
212201
public override bool Equals(object? obj) => obj is CommandProviderWrapper wrapper && isValid == wrapper.isValid;
213202

214203
public override int GetHashCode() => _commandProvider.GetHashCode();

src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/CommandSettingsViewModel.cs

Lines changed: 32 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -2,18 +2,25 @@
22
// The Microsoft Corporation licenses this file to you under the MIT license.
33
// See the LICENSE file in the project root for more information.
44

5+
using ManagedCommon;
56
using Microsoft.CmdPal.UI.ViewModels.Models;
67
using Microsoft.CommandPalette.Extensions;
78

89
namespace Microsoft.CmdPal.UI.ViewModels;
910

10-
public partial class CommandSettingsViewModel(ICommandSettings _unsafeSettings, CommandProviderWrapper provider, TaskScheduler mainThread)
11+
public partial class CommandSettingsViewModel(ICommandSettings? _unsafeSettings, CommandProviderWrapper provider, TaskScheduler mainThread)
1112
{
1213
private readonly ExtensionObject<ICommandSettings> _model = new(_unsafeSettings);
1314

1415
public ContentPageViewModel? SettingsPage { get; private set; }
1516

16-
public void InitializeProperties()
17+
public bool Initialized { get; private set; }
18+
19+
public bool HasSettings =>
20+
_model.Unsafe != null && // We have a settings model AND
21+
(!Initialized || SettingsPage != null); // we weren't initialized, OR we were, and we do have a settings page
22+
23+
private void UnsafeInitializeProperties()
1724
{
1825
var model = _model.Unsafe;
1926
if (model == null)
@@ -27,4 +34,27 @@ public void InitializeProperties()
2734
SettingsPage.InitializeProperties();
2835
}
2936
}
37+
38+
public void SafeInitializeProperties()
39+
{
40+
try
41+
{
42+
UnsafeInitializeProperties();
43+
}
44+
catch (Exception ex)
45+
{
46+
Logger.LogError($"Failed to load settings page", ex: ex);
47+
}
48+
49+
Initialized = true;
50+
}
51+
52+
public void DoOnUiThread(Action action)
53+
{
54+
Task.Factory.StartNew(
55+
action,
56+
CancellationToken.None,
57+
TaskCreationOptions.None,
58+
mainThread);
59+
}
3060
}

src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/ProviderSettingsViewModel.cs

Lines changed: 81 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,8 @@ public partial class ProviderSettingsViewModel(
1818
IServiceProvider _serviceProvider) : ObservableObject
1919
{
2020
private readonly SettingsModel _settings = _serviceProvider.GetService<SettingsModel>()!;
21+
private readonly Lock _initializeSettingsLock = new();
22+
private Task? _initializeSettingsTask;
2123

2224
public string DisplayName => _provider.DisplayName;
2325

@@ -34,6 +36,9 @@ public partial class ProviderSettingsViewModel(
3436

3537
public IconInfoViewModel Icon => _provider.Icon;
3638

39+
[ObservableProperty]
40+
public partial bool LoadingSettings { get; set; } = _provider.Settings?.HasSettings ?? false;
41+
3742
public bool IsEnabled
3843
{
3944
get => _providerSettings.IsEnabled;
@@ -56,15 +61,60 @@ public bool IsEnabled
5661
}
5762
}
5863

59-
private void Provider_CommandsChanged(CommandProviderWrapper sender, CommandPalette.Extensions.IItemsChangedEventArgs args)
64+
/// <summary>
65+
/// Gets a value indicating whether returns true if we have a settings page
66+
/// that's initialized, or we are still working on initializing that
67+
/// settings page. If we don't have a settings object, or that settings
68+
/// object doesn't have a settings page, then we'll return false.
69+
/// </summary>
70+
public bool HasSettings
6071
{
61-
OnPropertyChanged(nameof(ExtensionSubtext));
62-
OnPropertyChanged(nameof(TopLevelCommands));
72+
get
73+
{
74+
if (_provider.Settings == null)
75+
{
76+
return false;
77+
}
78+
79+
if (_provider.Settings.Initialized)
80+
{
81+
return _provider.Settings.HasSettings;
82+
}
83+
84+
// settings still need to be loaded.
85+
return LoadingSettings;
86+
}
6387
}
6488

65-
public bool HasSettings => _provider.Settings != null && _provider.Settings.SettingsPage != null;
89+
/// <summary>
90+
/// Gets will return the settings page, if we have one, and have initialized it.
91+
/// If we haven't initialized it, this will kick off a thread to start
92+
/// initializing it.
93+
/// </summary>
94+
public ContentPageViewModel? SettingsPage
95+
{
96+
get
97+
{
98+
if (_provider.Settings == null)
99+
{
100+
return null;
101+
}
66102

67-
public ContentPageViewModel? SettingsPage => HasSettings ? _provider?.Settings?.SettingsPage : null;
103+
if (_provider.Settings.Initialized)
104+
{
105+
LoadingSettings = false;
106+
return _provider.Settings.SettingsPage;
107+
}
108+
109+
// Don't load the settings if we're already working on it
110+
lock (_initializeSettingsLock)
111+
{
112+
_initializeSettingsTask ??= Task.Run(InitializeSettingsPage);
113+
}
114+
115+
return null;
116+
}
117+
}
68118

69119
[field: AllowNull]
70120
public List<TopLevelViewModel> TopLevelCommands
@@ -90,4 +140,30 @@ private List<TopLevelViewModel> BuildTopLevelViewModels()
90140
}
91141

92142
private void Save() => SettingsModel.SaveSettings(_settings);
143+
144+
private void InitializeSettingsPage()
145+
{
146+
if (_provider.Settings == null)
147+
{
148+
return;
149+
}
150+
151+
_provider.Settings.SafeInitializeProperties();
152+
_provider.Settings.DoOnUiThread(() =>
153+
{
154+
// Changing these properties will try to update XAML, and that has
155+
// to be handled on the UI thread, so we need to raise them on the
156+
// UI thread
157+
LoadingSettings = false;
158+
OnPropertyChanged(nameof(HasSettings));
159+
OnPropertyChanged(nameof(LoadingSettings));
160+
OnPropertyChanged(nameof(SettingsPage));
161+
});
162+
}
163+
164+
private void Provider_CommandsChanged(CommandProviderWrapper sender, CommandPalette.Extensions.IItemsChangedEventArgs args)
165+
{
166+
OnPropertyChanged(nameof(ExtensionSubtext));
167+
OnPropertyChanged(nameof(TopLevelCommands));
168+
}
93169
}

src/modules/cmdpal/Microsoft.CmdPal.UI.ViewModels/TopLevelCommandManager.cs

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,9 @@ public TopLevelCommandManager(IServiceProvider serviceProvider)
4545

4646
public async Task<bool> LoadBuiltinsAsync()
4747
{
48+
var s = new Stopwatch();
49+
s.Start();
50+
4851
_builtInCommands.Clear();
4952

5053
// Load built-In commands first. These are all in-proc, and
@@ -64,6 +67,10 @@ public async Task<bool> LoadBuiltinsAsync()
6467
}
6568
}
6669

70+
s.Stop();
71+
72+
Logger.LogDebug($"Loading built-ins took {s.ElapsedMilliseconds}ms");
73+
6774
return true;
6875
}
6976

src/modules/cmdpal/Microsoft.CmdPal.UI/Settings/ExtensionPage.xaml

Lines changed: 18 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -113,7 +113,24 @@
113113
Visibility="{x:Bind ViewModel.HasSettings}" />
114114

115115
<Frame x:Name="SettingsFrame" Visibility="{x:Bind ViewModel.HasSettings}">
116-
<cmdpalUI:ContentPage ViewModel="{x:Bind ViewModel.SettingsPage, Mode=OneWay}" />
116+
117+
<controls:SwitchPresenter
118+
HorizontalAlignment="Stretch"
119+
TargetType="x:Boolean"
120+
Value="{x:Bind ViewModel.LoadingSettings, Mode=OneWay}">
121+
<controls:Case Value="True">
122+
<ProgressRing
123+
Width="36"
124+
Height="36"
125+
HorizontalAlignment="Center"
126+
VerticalAlignment="Center"
127+
IsIndeterminate="True" />
128+
</controls:Case>
129+
<controls:Case Value="False">
130+
<cmdpalUI:ContentPage ViewModel="{x:Bind ViewModel.SettingsPage, Mode=OneWay}" />
131+
</controls:Case>
132+
</controls:SwitchPresenter>
133+
117134
</Frame>
118135

119136
<TextBlock

0 commit comments

Comments
 (0)