-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Lazy load the services needed by UsageTracker. #668
Conversation
To improve the startup time of the extension.
//var usageTracker = await GetServiceAsync(typeof(IUsageTracker)) as IUsageTracker; | ||
//usageTracker.IncrementLaunchCount(); | ||
// Activate the usage tracker by forcing an instance to be created. | ||
await GetServiceAsync(typeof(IUsageTracker)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We probably don't have to wait for it to be done, could we just fire and forget like we do with the menus?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, we can.
firstRun = false; | ||
} | ||
|
||
if (userSettings == null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you're calling Initialize
above, then this condition will always be false, right? Do we need any of the code inside this if
block?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The userSettings
check is unnecessary, you're right. However we still need to stop the timer if client == null
. I think you added the SwitchToMainThreadAsync
there - is that still necessary? Not sure why it's needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Because this might get called from a background thread (if it was triggered from a background thread) and we were doing GetExportedValue
, which needs to be called on the main thread. Speaking of which, we need to be very careful about what we call in Initialize
, there's side effects that can deadlock, I'll comment there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well TimerTick
will only ever get called on the main thread as it's triggered from a DispatcherTimer
, so it was unnecessary before. But you're right that it's necessary now as Initialize
may be called on a b/g thread from one of the IncrementX
methods.
client = uiProvider.GetService<IMetricsService>(); | ||
connectionManager = uiProvider.GetService<IConnectionManager>(); | ||
userSettings = uiProvider.GetService<IPackageSettings>(); | ||
vsservices = uiProvider.GetService<IVSServices>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any or all of these things can trigger a MEF call via GetExportedValue
, so we need to make sure this is done from the main thread, not a background thread. We'll need await ThreadHelper.JoinableTaskFactory.SwitchToMainThreadAsync();
before these calls, GetService
is not thread-safe.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right, the timer is a DispatcherTimer
so the tick method will always call this from the main thread so that's fine. I don't think anything else will be calling this from background threads, but in case that happens, probably best to make sure we're on the right thread here.
Required changing most of the UsageTracker methods to be async.
✨ |
To improve the startup time of the extension.