SF-3875 Improve note sync performance

src/SIL.XForge.Scripture/Services/CommentManagerExtensions.cs

@@ -0,0 +1,57 @@
+using System;
+using System.Collections.Generic;
+using System.Linq;
+using Paratext.Data.ProjectComments;
+
+namespace SIL.XForge.Scripture.Services;
+
+/// <summary> Extension methods for ParatextData's <see cref="CommentManager"/>. </summary>
+public static class CommentManagerExtensions
+{
+    /// <summary>
+    /// Finds comment threads, like
+    /// <see cref="CommentManager.FindThreads(Func{CommentThread, bool}, bool, bool)"/>, but with
+    /// every thread guaranteed to be complete. FindThreads sorts all comments and then groups only
+    /// adjacent comments that share a thread id. Its sort order is undefined for a thread whose
+    /// comments do not all share the same anchor, so FindThreads can return such a thread split
+    /// into fragments that share an id. (Biblical-term and spelling-note threads have this shape:
+    /// their location-independent ids, such as BT_term and project_word, collect comments
+    /// anchored at different verses.) Instead, build every thread the way
+    /// <see cref="CommentManager.FindThread(string)"/> does (select comments by thread id and
+    /// sort them), but in one pass over the comments rather than one full scan per thread.
+    /// Threads are returned ordered by their oldest comment's anchor. The filters are applied to
+    /// the complete threads.
+    /// </summary>
+    public static List<CommentThread> FindCompleteThreads(
+        this CommentManager manager,
+        Func<CommentThread, bool>? shouldThreadBeIncluded = null,
+        bool activeOnly = false
+    )
+    {
+        // Unlike FindThreads and FindThread, AllComments does not take the manager's lock, so
+        // this must not run concurrently with mutations of the same project's comments.
+        Dictionary<string, CommentThread> threadsById = [];
+        foreach (Comment comment in manager.AllComments)
+        {
+            if (!threadsById.TryGetValue(comment.Thread, out CommentThread? thread))
+            {
+                thread = new CommentThread { ScrText = manager.ScrText };
+                threadsById[comment.Thread] = thread;
+            }
+            thread.Comments.Add(comment);
+        }
+        List<CommentThread> threads = [.. threadsById.Values];
+        // Comment.CompareTo compares same-thread comments by date and different-thread comments
+        // by anchor, so this sorts each thread's comments oldest first (as FindThread does) and
+        // then orders the threads by the anchor of each thread's oldest comment.
+        foreach (CommentThread thread in threads)
+            thread.Comments.Sort();
+        threads.Sort((a, b) => a.Comments[0].CompareTo(b.Comments[0]));
+        IEnumerable<CommentThread> result = threads;
+        if (activeOnly)
+            result = result.Where(t => t.Comments.Any(c => !c.Deleted));
+        if (shouldThreadBeIncluded is not null)
+            result = result.Where(shouldThreadBeIncluded);
+        return [.. result];
+    }
+}

src/SIL.XForge.Scripture/Services/ParatextDataHelper.cs

@@ -42,7 +42,7 @@ public void CommitVersionedText(ScrText scrText, string comment)
     public IReadOnlyList<ParatextNote> GetNotes(CommentManager commentManager, CommentTags commentTags)
     {
         // Only return note threads that are not resolved and active
-        IEnumerable<CommentThread> threads = commentManager.FindThreads(
+        IEnumerable<CommentThread> threads = commentManager.FindCompleteThreads(
             t => t.Status != NoteStatus.Resolved,
             activeOnly: true
         );

src/SIL.XForge.Scripture/Services/ParatextService.cs

@@ -1334,10 +1334,10 @@ public string GetNotes(UserSecret userSecret, string paratextId, int bookNum)
 
         // CommentThread.VerseRef determines the location of a thread, even if moved. However, in Paratext a note can
         // only be relocated within the chapter, so for our query, we only need to look at the first note location.
-        var threads = manager.FindThreads(
+        var threads = manager.FindCompleteThreads(
             commentThread =>
                 commentThread.Comments[0].VerseRefStr.StartsWith(verseRefBook, StringComparison.OrdinalIgnoreCase),
-            true
+            activeOnly: true
         );
         return NotesFormatter.FormatNotes(threads);
     }
@@ -1372,6 +1372,13 @@ Dictionary<string, ParatextUserProfile> ptProjectUsers
             nt.Data.Notes.Any(n => !n.Deleted)
         );
 
+        // FindThread(id) answers by scanning the project's entire comment list, so calling it for
+        // each of the project's threads costs one full scan per thread (minutes of work at tens
+        // of thousands of threads). Instead, get every thread in one pass and index them by id.
+        Dictionary<string, CommentThread> commentThreadsById = commentManager
+            .FindCompleteThreads()
+            .ToDictionary(t => t.Id);
+
         foreach (var threadDoc in activeNoteThreadDocs)
         {
             List<string> matchedCommentIds = [];
@@ -1388,7 +1395,7 @@ Dictionary<string, ParatextUserProfile> ptProjectUsers
                 threadDoc.Data.ExtraHeadingInfo
             );
             // Find the corresponding comment thread
-            CommentThread? existingThread = commentManager.FindThread(threadDoc.Data.ThreadId);
+            commentThreadsById.TryGetValue(threadDoc.Data.ThreadId, out CommentThread? existingThread);
             if (existingThread is null)
             {
                 // The thread has been removed
@@ -1462,7 +1469,7 @@ Dictionary<string, ParatextUserProfile> ptProjectUsers
         IEnumerable<string> newThreadIds = ptThreadIds.Except(matchedThreadIds);
         foreach (string threadId in newThreadIds)
         {
-            CommentThread? thread = commentManager.FindThread(threadId);
+            commentThreadsById.TryGetValue(threadId, out CommentThread? thread);
             if (thread is null || thread.Comments.All(c => c.Deleted))
                 continue;
             Paratext.Data.ProjectComments.Comment info = thread.Comments[0];
@@ -2989,17 +2996,13 @@ private static IEnumerable<CommentThread> GetCommentThreads(CommentManager manag
         // reallocated within the chapter, so for our query, we only need the first location.
         // A Biblical Term has a VerseRef, but it is usually not useful, so we exclude BT notes when getting a book's notes
         // The VerseRef will still be stored for a BT note, as this is a PT requirement.
-        return manager.FindThreads(
-            commentThread =>
-                (
-                    bookNum != null
-                    && commentThread
-                        .Comments[0]
-                        .VerseRefStr.StartsWith(verseRefBook, StringComparison.OrdinalIgnoreCase)
-                    && !commentThread.IsBTNote
-                    && !commentThread.Id.StartsWith("ANSWER_")
-                ) || (bookNum == null && commentThread.IsBTNote),
-            false
+        return manager.FindCompleteThreads(commentThread =>
+            (
+                bookNum != null
+                && commentThread.Comments[0].VerseRefStr.StartsWith(verseRefBook, StringComparison.OrdinalIgnoreCase)
+                && !commentThread.IsBTNote
+                && !commentThread.Id.StartsWith("ANSWER_")
+            ) || (bookNum == null && commentThread.IsBTNote)
         );
     }
 

test/SIL.XForge.Scripture.Tests/Services/CommentManagerExtensionsTests.cs

@@ -0,0 +1,156 @@
+using System;
+using System.Collections.Generic;
+using System.IO;
+using System.Linq;
+using System.Xml;
+using NSubstitute;
+using NUnit.Framework;
+using Paratext.Data;
+using Paratext.Data.Languages;
+using Paratext.Data.ProjectComments;
+using Paratext.Data.ProjectFileAccess;
+using Paratext.Data.Repository;
+using SIL.WritingSystems;
+using SIL.XForge.Scripture.Models;
+using ParatextComment = Paratext.Data.ProjectComments.Comment;
+
+namespace SIL.XForge.Scripture.Services;
+
+[TestFixture]
+public class CommentManagerExtensionsTests
+{
+    private const string ParatextUser01 = "ParatextUser01";
+
+    // A thread whose comments do not all share the same anchor makes Comment.CompareTo
+    // inconsistent: comments of the same thread compare to each other by date, but to other
+    // threads' comments by anchor. Biblical-term and spelling-note threads have this shape:
+    // their location-independent ids (BT_term, project_word) collect comments anchored at
+    // different verses. CommentManager.FindThreads sorts all
+    // comments and then groups only adjacent comments that share a thread id, so an unlucky sort
+    // order returns such a thread split into fragments. This arrangement makes the split
+    // deterministic: each comment compares greater than the one before it, so List<T>.Sort's
+    // insertion sort (used for small lists) keeps this exact order, leaving thread-a's two
+    // comments separated by thread-b's. thread-a's second comment anchors after thread-b's
+    // comment but is dated before its own thread-mate, which is where the inconsistency bites.
+    private static readonly (string Thread, string VerseRef, DateTimeOffset Date)[] SplitThreadArrangement =
+    [
+        ("thread-a", "RUT 1:1", new DateTimeOffset(2020, 1, 1, 0, 0, 0, TimeSpan.Zero)),
+        ("thread-b", "RUT 1:2", new DateTimeOffset(2020, 1, 2, 0, 0, 0, TimeSpan.Zero)),
+        ("thread-a", "RUT 1:3", new DateTimeOffset(2019, 1, 1, 0, 0, 0, TimeSpan.Zero)),
+        ("thread-c", "RUT 1:4", new DateTimeOffset(2020, 1, 3, 0, 0, 0, TimeSpan.Zero)),
+    ];
+
+    [Test]
+    public void FindThreads_SplitsAThreadWhoseCommentsHaveDifferentAnchors()
+    {
+        var env = new TestEnvironment();
+        using MockScrText scrText = env.GetScrText(HexId.CreateNew().ToString());
+        CommentManager manager = CommentManager.Get(scrText);
+        foreach ((string thread, string verseRef, DateTimeOffset date) in SplitThreadArrangement)
+            TestEnvironment.AddComment(scrText, thread, verseRef, date);
+
+        // SUT
+        List<CommentThread> threads = manager.FindThreads();
+
+        // This asserts the ParatextData bug that FindCompleteThreads exists to work around:
+        // thread-a comes back as two single-comment fragments sharing an id. If this test fails
+        // after a ParatextData (or .NET sort) update because thread-a comes back whole, the
+        // workaround (and this test pair) can likely be removed.
+        Assert.That(threads.Count(t => t.Id == "thread-a"), Is.EqualTo(2));
+        Assert.That(threads.Where(t => t.Id == "thread-a").Select(t => t.Comments.Count), Is.All.EqualTo(1));
+    }
+
+    [Test]
+    public void FindCompleteThreads_ReturnsEveryThreadComplete()
+    {
+        var env = new TestEnvironment();
+        using MockScrText scrText = env.GetScrText(HexId.CreateNew().ToString());
+        CommentManager manager = CommentManager.Get(scrText);
+        foreach ((string thread, string verseRef, DateTimeOffset date) in SplitThreadArrangement)
+            TestEnvironment.AddComment(scrText, thread, verseRef, date);
+
+        // SUT
+        List<CommentThread> threads = manager.FindCompleteThreads();
+
+        Assert.That(threads.Select(t => t.Id), Is.EquivalentTo(new[] { "thread-a", "thread-b", "thread-c" }));
+        CommentThread threadA = threads.Single(t => t.Id == "thread-a");
+        // Complete, and in FindThread's order: oldest comment first
+        Assert.That(threadA.Comments.Select(c => c.VerseRefStr), Is.EqualTo(["RUT 1:3", "RUT 1:1"]));
+    }
+
+    [Test]
+    public void FindCompleteThreads_OrdersThreadsByOldestCommentAnchor()
+    {
+        var env = new TestEnvironment();
+        using MockScrText scrText = env.GetScrText(HexId.CreateNew().ToString());
+        CommentManager manager = CommentManager.Get(scrText);
+        foreach ((string thread, string verseRef, DateTimeOffset date) in SplitThreadArrangement)
+            TestEnvironment.AddComment(scrText, thread, verseRef, date);
+
+        // SUT
+        List<CommentThread> threads = manager.FindCompleteThreads();
+
+        // A thread is located at its oldest comment's anchor: thread-a's oldest comment (dated
+        // 2019) anchors at RUT 1:3, placing the thread between thread-b (RUT 1:2) and thread-c
+        // (RUT 1:4).
+        Assert.That(threads.Select(t => t.Id), Is.EqualTo(["thread-b", "thread-a", "thread-c"]));
+    }
+
+    private class TestEnvironment
+    {
+        private readonly string _syncDir = Path.GetTempPath();
+
+        public TestEnvironment()
+        {
+            // Ensure that the SLDR is initialized for LanguageID.Code to be retrieved correctly
+            if (!Sldr.IsInitialized)
+                Sldr.Initialize(true);
+
+            // Setup Mercurial for tests
+            Hg.DefaultRunnerCreationFunc = (_, _, _) => new MockHgRunner();
+            Hg.Default = new MockHg();
+            VersionedText.AllCommitsDisabled = true;
+        }
+
+        public static void AddComment(ScrText scrText, string threadId, string verseRef, DateTimeOffset date)
+        {
+            XmlDocument doc = new XmlDocument();
+            XmlElement root = doc.CreateElement("content");
+            XmlElement paragraph = doc.CreateElement("p");
+            paragraph.InnerText = "comment text";
+            root.AppendChild(paragraph);
+            doc.AppendChild(root);
+
+            var comment = new ParatextComment(scrText.User)
+            {
+                Thread = threadId,
+                VerseRefStr = verseRef,
+                Contents = root,
+                DateTime = date,
+                Status = NoteStatus.Todo,
+                SelectedText = string.Empty,
+                StartPosition = 0,
+            };
+            CommentManager.Get(scrText).AddComment(comment);
+        }
+
+        public MockScrText GetScrText(string paratextId)
+        {
+            string scrTextDir = Path.Join(_syncDir, paratextId, "target");
+            ProjectName projectName = new ProjectName { ProjectPath = scrTextDir, ShortName = "Proj" };
+            var scrText = new MockScrText(new SFParatextUser(ParatextUser01), projectName)
+            {
+                CachedGuid = HexId.FromStr(paratextId),
+            };
+            scrText.Settings.LanguageID = LanguageId.English;
+            scrText.Settings.FileNamePostPart = ".SFM";
+
+            // Set up the file manager for the comment manager
+            ProjectFileManager fileManager = Substitute.For<ProjectFileManager>(scrText, null);
+            fileManager.IsWritable.Returns(true);
+            scrText.SetFileManager(fileManager);
+
+            return scrText;
+        }
+    }
+}