SF-3875 Improve note sync performance - #4003
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #4003 +/- ##
=======================================
Coverage 81.12% 81.13%
=======================================
Files 659 660 +1
Lines 42631 42649 +18
Branches 6993 6992 -1
=======================================
+ Hits 34584 34602 +18
+ Misses 6908 6895 -13
- Partials 1139 1152 +13 ☔ View full report in Codecov by Harness. |
pmachapman
left a comment
There was a problem hiding this comment.
@Nateowami Has this bug been reported to the Paratext team? I can't actually think of a way of fixing the bug within Comment.CompareTo(), as it seems a grouping (which your code does) is needed to fix the sorting.
@pmachapman reviewed 4 files and all commit messages, and made 2 comments.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on Nateowami).
src/SIL.XForge.Scripture/Services/CommentManagerExtensions.cs line 41 at r1 (raw file):
threads.Add(thread); } }
We could speed this up by iterating over manager.AllComments, then building the CommentThreads from there. In particular, we would group then sort the comments correctly via our own version of the logic in CommentManager.SortAndGroupThreads.
I'm not making this blocking, as I am interested in your thoughts, although perhaps I lean towards us implementing our own iterator as the PT CommentManager.FindThreads() code has not changed in years?
Code quote:
foreach (CommentThread thread in manager.FindThreads())
{
if (threadIndexById.TryGetValue(thread.Id, out int index))
threads[index] = manager.FindThread(thread.Id);
else
{
threadIndexById[thread.Id] = threads.Count;
threads.Add(thread);
}
}- Performance improvement comes by looking up all note threads in a single pass and placing them in a dictionary - Also fixed a bug caused by CommentManager.FindThreads not properly grouping threads with comments on multiple verses.
192df5a to
f244bc6
Compare
Nateowami
left a comment
There was a problem hiding this comment.
@Nateowami made 1 comment and resolved 1 discussion.
Reviewable status: 2 of 4 files reviewed, all discussions resolved (waiting on pmachapman).
src/SIL.XForge.Scripture/Services/CommentManagerExtensions.cs line 41 at r1 (raw file):
Previously, pmachapman (Peter Chapman) wrote…
We could speed this up by iterating over
manager.AllComments, then building theCommentThreadsfrom there. In particular, we would group then sort the comments correctly via our own version of the logic inCommentManager.SortAndGroupThreads.I'm not making this blocking, as I am interested in your thoughts, although perhaps I lean towards us implementing our own iterator as the PT
CommentManager.FindThreads()code has not changed in years?
Done.
CommentManager.FindThreadsnot properly grouping threads with comments on multiple versesProjects with tons of threads take a long time to sync, and I think it grows with
[thread count] * [comments count across all threads]. This results in some very slow syncs that prevent other projects from being able to sync, since we only allow one sync at a time (the worst offender on live spent 17min 18s just on note threads).The bigger change on the PR is wrapping
CommentManager.FindThreadsto handle cases where it emits the same thread ID twice, along with a test to demonstrate a) that it does this with certain threads, and b) that the wrapper handles that scenario.This change is