Merge pull request #52 from NoiTheCat/dev/tweaks

Several performance tweaks

BackgroundServices/AutoUserDownload.cs

@@ -6,10 +6,18 @@ namespace BirthdayBot.BackgroundServices;
 /// Proactively fills the user cache for guilds in which any birthday data already exists.
 /// </summary>
 class AutoUserDownload : BackgroundService {
-    public AutoUserDownload(ShardInstance instance) : base(instance) { }
+    private static readonly TimeSpan RequestTimeout = ShardManager.DeadShardThreshold / 3;
 
-    private static readonly HashSet<ulong> _failedDownloads = new();
+    private readonly HashSet<ulong> _skippedGuilds = new();
-    private static readonly TimeSpan _singleDlTimeout = ShardManager.DeadShardThreshold / 3;
+
+    public AutoUserDownload(ShardInstance instance) : base(instance)
+        => Shard.DiscordClient.Disconnected += OnDisconnect;
+    ~AutoUserDownload() => Shard.DiscordClient.Disconnected -= OnDisconnect;
+
+    private Task OnDisconnect(Exception ex) {
+        _skippedGuilds.Clear();
+        return Task.CompletedTask;
+    }
 
     public override async Task OnTick(int tickCount, CancellationToken token) {
         // Take action if a guild's cache is incomplete...
@@ -22,13 +30,11 @@ class AutoUserDownload : BackgroundService {
         try {
             await ConcurrentSemaphore.WaitAsync(token);
             using var db = new BotDatabaseContext();
-            lock (_failedDownloads)
             mustFetch = db.UserEntries.AsNoTracking()
                 .Where(e => incompleteCaches.Contains(e.GuildId))
                 .Select(e => e.GuildId)
-                    .Distinct()
+                .Where(e => !_skippedGuilds.Contains(e))
-                    .Where(e => !_failedDownloads.Contains(e))
+                .ToHashSet();
-                    .ToList();
         } finally {
             try {
                 ConcurrentSemaphore.Release();
@@ -38,29 +44,58 @@ class AutoUserDownload : BackgroundService {
         var processed = 0;
         var processStartTime = DateTimeOffset.UtcNow;
         foreach (var item in mustFetch) {
-            // May cause a disconnect in certain situations. Make no further attempts until the next pass if it happens.
+            // Take break from processing to avoid getting killed by ShardManager
+            if (DateTimeOffset.UtcNow - processStartTime > RequestTimeout) break;
+
+            // We're useless if not connected
             if (Shard.DiscordClient.ConnectionState != ConnectionState.Connected) break;
 
             var guild = Shard.DiscordClient.GetGuild(item);
             if (guild == null) continue; // A guild disappeared...?
 
-            await Task.Delay(200, CancellationToken.None); // Delay a bit (reduces the possibility of hanging, somehow).
             processed++;
+
+            await Task.Delay(200, CancellationToken.None); // Delay a bit (reduces the possibility of hanging, somehow).
             var dl = guild.DownloadUsersAsync();
-            dl.Wait((int)_singleDlTimeout.TotalMilliseconds / 2, token);
+            try {
+                dl.Wait((int)RequestTimeout.TotalMilliseconds / 2, token);
+            } catch (Exception) { }
+            if (token.IsCancellationRequested) return; // Skip all reporting, error logging on cancellation
+
             if (dl.IsFaulted) {
                 Log("Exception thrown by download task: " + dl.Exception);
                 break;
             } else if (!dl.IsCompletedSuccessfully) {
-                Log($"Task for guild {guild.Id} is unresponsive. Skipping guild. Members: {guild.MemberCount}. Name: {guild.Name}.");
+                Log($"Task unresponsive, will skip (ID {guild.Id}, with {guild.MemberCount} members).");
-                lock (_failedDownloads) _failedDownloads.Add(guild.Id);
+                _skippedGuilds.Add(guild.Id);   
                 continue;
             }
-
-            // Prevent unnecessary disconnections by ShardManager if we're taking too long
-            if (DateTimeOffset.UtcNow - processStartTime > _singleDlTimeout) break;
         }
 
         if (processed > 10) Log($"Member list downloads handled for {processed} guilds.");
+        ConsiderGC(processed);
     }
+
+    #region Manual garbage collection
+    private static readonly object _mgcTrackLock = new();
+    private static int _mgcProcessedSinceLast = 0;
+
+    // Downloading user information adds up memory-wise, particularly within the
+    // Gen 2 collection. Here we attempt to balance not calling the GC too much
+    // while also avoiding dying to otherwise inevitable excessive memory use.
+    private static void ConsiderGC(int processed) {
+        const int CallGcAfterProcessingAmt = 1500;
+        bool trigger;
+        lock (_mgcTrackLock) {
+            _mgcProcessedSinceLast += processed;
+            trigger = _mgcProcessedSinceLast > CallGcAfterProcessingAmt;
+            if (trigger) _mgcProcessedSinceLast = 0;
+        }
+        if (trigger) {
+            Program.Log(nameof(AutoUserDownload), "Invoking garbage collection...");
+            GC.Collect(2, GCCollectionMode.Forced, true, true);
+            Program.Log(nameof(AutoUserDownload), "Complete.");
+        }
+    }
+    #endregion
 }

BackgroundServices/BirthdayRoleUpdate.cs

@@ -15,8 +15,8 @@ class BirthdayRoleUpdate : BackgroundService {
     /// </summary>
     public override async Task OnTick(int tickCount, CancellationToken token) {
         try {
-            await ConcurrentSemaphore.WaitAsync(token);
+            await ConcurrentSemaphore.WaitAsync(token).ConfigureAwait(false);
-            await ProcessBirthdaysAsync(token);
+            await ProcessBirthdaysAsync(token).ConfigureAwait(false);
         } finally {
             try {
                 ConcurrentSemaphore.Release();
@@ -25,7 +25,7 @@ class BirthdayRoleUpdate : BackgroundService {
     }
 
     private async Task ProcessBirthdaysAsync(CancellationToken token) {
-        // For database efficiency, fetch all database information at once before proceeding
+        // For database efficiency, fetch all pertinent 'global' database information at once before proceeding
         using var db = new BotDatabaseContext();
         var shardGuilds = Shard.DiscordClient.Guilds.Select(g => g.Id).ToHashSet();
         var presentGuildSettings = db.GuildConfigurations.Where(s => shardGuilds.Contains(s.GuildId));
@@ -39,35 +39,35 @@ class BirthdayRoleUpdate : BackgroundService {
             // Check task cancellation here. Processing during a single guild is never interrupted.
             if (token.IsCancellationRequested) throw new TaskCanceledException();
 
-            if (Shard.DiscordClient.ConnectionState != ConnectionState.Connected) {
+            // Stop if we've disconnected.
-                Log("Client is not connected. Stopping early.");
+            if (Shard.DiscordClient.ConnectionState != ConnectionState.Connected) break;
-                return;
-            }
 
             try {
                 // Verify that role settings and permissions are usable
-                SocketRole? role = guild.GetRole((ulong)(settings.BirthdayRole ?? 0));
+                SocketRole? role = guild.GetRole(settings.BirthdayRole ?? 0);
-                if (role == null
+                if (role == null) continue; // Role not set.
-                    || !guild.CurrentUser.GuildPermissions.ManageRoles
+                if (!guild.CurrentUser.GuildPermissions.ManageRoles || role.Position >= guild.CurrentUser.Hierarchy) {
-                    || role.Position >= guild.CurrentUser.Hierarchy) continue;
+                    // Quit this guild if insufficient role permissions.
+                    continue;
+                }
                 if (role.IsEveryone || role.IsManaged) {
                     // Invalid role was configured. Clear the setting and quit.
                     settings.BirthdayRole = null;
                     db.Update(settings);
-                    await db.SaveChangesAsync(CancellationToken.None);
+                    await db.SaveChangesAsync(CancellationToken.None).ConfigureAwait(false);
                     continue;
                 }
 
                 // Load up user configs and begin processing birthdays
-                await db.Entry(settings).Collection(t => t.UserEntries).LoadAsync(CancellationToken.None);
+                await db.Entry(settings).Collection(t => t.UserEntries).LoadAsync(CancellationToken.None).ConfigureAwait(false);
                 var birthdays = GetGuildCurrentBirthdays(settings.UserEntries, settings.GuildTimeZone);
 
                 // Add or remove roles as appropriate
-                var announcementList = await UpdateGuildBirthdayRoles(guild, role, birthdays);
+                var announcementList = await UpdateGuildBirthdayRoles(guild, role, birthdays).ConfigureAwait(false);
 
                 // Process birthday announcement
                 if (announcementList.Any()) {
-                    await AnnounceBirthdaysAsync(settings, guild, announcementList);
+                    await AnnounceBirthdaysAsync(settings, guild, announcementList).ConfigureAwait(false);
                 }
             } catch (Exception ex) {
                 // Catch all exceptions per-guild but continue processing, throw at end.
@@ -93,9 +93,9 @@ class BirthdayRoleUpdate : BackgroundService {
             var checkNow = SystemClock.Instance.GetCurrentInstant().InZone(tz);
             // Special case: If user's birthday is 29-Feb and it's currently not a leap year, check against 1-Mar
             if (!DateTime.IsLeapYear(checkNow.Year) && record.BirthMonth == 2 && record.BirthDay == 29) {
-                if (checkNow.Month == 3 && checkNow.Day == 1) birthdayUsers.Add((ulong)record.UserId);
+                if (checkNow.Month == 3 && checkNow.Day == 1) birthdayUsers.Add(record.UserId);
             } else if (record.BirthMonth == checkNow.Month && record.BirthDay== checkNow.Day) {
-                birthdayUsers.Add((ulong)record.UserId);
+                birthdayUsers.Add(record.UserId);
             }
         }
 
@@ -120,14 +120,14 @@ class BirthdayRoleUpdate : BackgroundService {
                 else no_ops.Add(user.Id);
             }
             foreach (var user in removals) {
-                await user.RemoveRoleAsync(r);
+                await user.RemoveRoleAsync(r).ConfigureAwait(false);
             }
 
             foreach (var target in toApply) {
                 if (no_ops.Contains(target)) continue;
                 var user = g.GetUser(target);
                 if (user == null) continue; // User existing in database but not in guild
-                await user.AddRoleAsync(r);
+                await user.AddRoleAsync(r).ConfigureAwait(false);
                 additions.Add(user);
             }
         } catch (Discord.Net.HttpException ex)
@@ -144,7 +144,7 @@ class BirthdayRoleUpdate : BackgroundService {
     /// Attempts to send an announcement message.
     /// </summary>
     internal static async Task AnnounceBirthdaysAsync(GuildConfig settings, SocketGuild g, IEnumerable<SocketGuildUser> names) {
-        var c = g.GetTextChannel((ulong)(settings.AnnouncementChannel ?? 0));
+        var c = g.GetTextChannel(settings.AnnouncementChannel ?? 0);
         if (c == null) return;
         if (!c.Guild.CurrentUser.GetPermissions(c).SendMessages) return;
 

BackgroundServices/DataRetention.cs

@@ -14,11 +14,11 @@ class DataRetention : BackgroundService {
     const int StaleUserThreashold = 360;
 
     public DataRetention(ShardInstance instance) : base(instance) {
-        ProcessInterval = 5400 / Shard.Config.BackgroundInterval; // Process about once per hour and a half
+        ProcessInterval = 21600 / Shard.Config.BackgroundInterval; // Process about once per six hours
     }
 
     public override async Task OnTick(int tickCount, CancellationToken token) {
-        // On each tick, run only a set group of guilds, each group still processed every ProcessInterval ticks.
+        // Run only a subset of shards each time, each running every ProcessInterval ticks.
         if ((tickCount + Shard.ShardId) % ProcessInterval != 0) return;
 
         try {

ShardInstance.cs

@@ -94,7 +94,8 @@ public sealed class ShardInstance : IDisposable {
                     return Task.CompletedTask;
             }
 
-            Log("Discord.Net exception", arg.Exception.ToString());
+            if (arg.Exception is TaskCanceledException) return Task.CompletedTask; // We don't ever need to know these...
+            Log("Discord.Net exception", $"{arg.Exception.GetType().FullName}: {arg.Exception.Message}");
         }
 
         return Task.CompletedTask;

ShardManager.cs

@@ -43,8 +43,7 @@ class ShardManager : IDisposable {
 
         // Start status reporting thread
         _mainCancel = new CancellationTokenSource();
-        _statusTask = Task.Factory.StartNew(StatusLoop, _mainCancel.Token,
+        _statusTask = Task.Factory.StartNew(StatusLoop, _mainCancel.Token);
-                                            TaskCreationOptions.LongRunning, TaskScheduler.Default);
     }
 
     public void Dispose() {
@@ -62,8 +61,6 @@ class ShardManager : IDisposable {
         if (!Task.WhenAll(shardDisposes).Wait(30000)) {
             Log("Warning: Not all shards terminated cleanly after 30 seconds. Continuing...");
         }
-
-        Log($"Uptime: {Program.BotUptime}");
     }
 
     private void Log(string message) => Program.Log(nameof(ShardManager), message);