SF-3743 Fix checking slowdown when a project has many questions#3754
SF-3743 Fix checking slowdown when a project has many questions#3754pmachapman wants to merge 4 commits intomasterfrom
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #3754 +/- ##
==========================================
+ Coverage 81.32% 81.34% +0.01%
==========================================
Files 620 620
Lines 39086 39119 +33
Branches 6375 6349 -26
==========================================
+ Hits 31787 31821 +34
+ Misses 6329 6328 -1
Partials 970 970 ☔ View full report in Codecov by Sentry. |
|
My testing confirms a dramatic performance improvement. Thanks for tracking this down. IndexedDB indexes weren't even on my radar. |
RaymondLuong3
left a comment
There was a problem hiding this comment.
I was able to see the benefit locally for importing questions. I tested some of the other functionality for community checking and it looks like change detection is not working when playing audio. When I record an play audio, the progress is not updating. Can you look into this?
@RaymondLuong3 reviewed 5 files and all commit messages, and made 3 comments.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on pmachapman).
src/SIL.XForge.Scripture/ClientApp/src/app/checking/checking-overview/checking-overview.component.ts line 476 at r2 (raw file):
dialogRef.afterClosed().subscribe(() => { this.changeDetector.reattach(); this.changeDetector.markForCheck();
I noticed this when I was importing the questions and wondered if that was part of the change. Personally I think this makes sense since the dialog is open so we do not necessary need to be updating the calling component in the background
Code quote:
dialogRef.afterClosed().subscribe(() => {
this.changeDetector.reattach();
this.changeDetector.markForCheck();src/SIL.XForge.Scripture/ClientApp/src/app/core/models/question-doc.ts line 25 at r2 (raw file):
[obj<Question>().pathStr(n => n.projectRef)]: 1, [obj<Question>().pathStr(n => n.verseRef.bookNum)]: 1, [obj<Question>().pathStr(n => n.verseRef.chapterNum)]: 1
So this makes it easier for loading questions by book and chapter because these values are now indexed, is that correct? Does this benefit both in the context of querying for all questions in a book and for a book and chapter? I could not tell based on my testing.
Code quote:
[obj<Question>().pathStr(n => n.projectRef)]: 1,
[obj<Question>().pathStr(n => n.verseRef.bookNum)]: 1,
[obj<Question>().pathStr(n => n.verseRef.chapterNum)]: 1
pmachapman
left a comment
There was a problem hiding this comment.
When I record an play audio, the progress is not updating. Can you look into this?
Done. Thank you!
@pmachapman made 3 comments.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on RaymondLuong3).
src/SIL.XForge.Scripture/ClientApp/src/app/checking/checking-overview/checking-overview.component.ts line 476 at r2 (raw file):
Previously, RaymondLuong3 (Raymond Luong) wrote…
I noticed this when I was importing the questions and wondered if that was part of the change. Personally I think this makes sense since the dialog is open so we do not necessary need to be updating the calling component in the background
It helped the speed of the import to not refresh the background list of questions so often while an import was occurring. It does still happen due to other change detection updates, but at a much lower rate.
src/SIL.XForge.Scripture/ClientApp/src/app/core/models/question-doc.ts line 25 at r2 (raw file):
Previously, RaymondLuong3 (Raymond Luong) wrote…
So this makes it easier for loading questions by book and chapter because these values are now indexed, is that correct? Does this benefit both in the context of querying for all questions in a book and for a book and chapter? I could not tell based on my testing.
Yes. The index will be used if you query by book and chapter, or book, or chapter.
...e.Scripture/ClientApp/src/app/checking/checking-overview/checking-overview.component.spec.ts
Outdated
Show resolved
Hide resolved
...XForge.Scripture/ClientApp/src/app/checking/checking-overview/checking-overview.component.ts
Outdated
Show resolved
Hide resolved
src/SIL.XForge.Scripture/ClientApp/src/app/checking/checking/checking.component.ts
Show resolved
Hide resolved
...e.Scripture/ClientApp/src/app/checking/checking-overview/checking-overview.component.spec.ts
Show resolved
Hide resolved
RaymondLuong3
left a comment
There was a problem hiding this comment.
Thanks! I am still seeing some issues. When I record an answer and stop the recording, the change detection doesn't appear to run so that the audio is not shown as recorded before saving.
@RaymondLuong3 reviewed 4 files and all commit messages, made 2 comments, and resolved 1 discussion.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on pmachapman).
src/SIL.XForge.Scripture/ClientApp/src/app/core/models/question-doc.ts line 25 at r2 (raw file):
Previously, pmachapman (Peter Chapman) wrote…
Yes. The index will be used if you query by book and chapter, or book, or chapter.
Excellent!
RaymondLuong3
left a comment
There was a problem hiding this comment.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on pmachapman).
This PR addresses speed issue in community checking by adding an index to the questions in IndexedDB, and by modifying change detection to push for the checking component and checking overview component.
This change is