jimmejardine / qiqqa-open-source

The open-sourced version of the award-winning Qiqqa research management tool for Windows
GNU General Public License v3.0
373 stars 61 forks source link

Qiqqa performance: hold off background tasks until the application has completely rendered the UI #55

Closed GerHobbelt closed 4 years ago

GerHobbelt commented 5 years ago

Running background tasks after a fixed delay is okay, but with large libs on slow boxes (or when running Qiqqa in a development environment while running dev tasks, e.g. debugging or performance measuring) this initial delay is too short.

GerHobbelt commented 5 years ago

Progress... of sorts 🤔

I'm running into additional nastiness with this. The thread-safety edits I did to date are under suspicion as it smells like a deadlock somewhere but after instrumenting all lock (obj_lock) {...} code bits, it isn't entirely obvious where the waiting comes from -- or whether it's a deadlock issue at all: it looks like 1 core is about maxed out most of the time when starting Qiqqa and opening the 'guest lib' @ 29K documents.

However, the same 'dreadful long UI wait' happens when I move that huge library aside and let Qiqqa only load one 'evil library' @ 1 document only plus an old intranet lib @ 37 articles: this should be a snap but isn't with my codebase today.

Performance-wise there are a few things that start to come out of the many hours of intrumenting, debugging and performance analysis:

  1. on open, every control takes the entire library and traverses it to extract its own datums, be it tags, years of publication or otherwise, where a lot of locking and unlocking was/is going on as every document is fetched about 8-10 times (as there are that many controls doing this at the same time) via this Library API:

    public PDFDocument GetDocumentByFingerprint(string fingerprint)

    which has thread-safety via lock(...) {...} critical section coding built in. Executing about 9 x 29K+ lock/unlock ops isn't zero cost either.

  2. Then there's this bit of code in LibraryFilterController which is executed on lib open and causes every bloody control to run the entire lib through its own flavor of GetNodeItems library traversal loop code (which uses that GetDocumentByFingerprint API mentioned above almost everywhere):

    public Library Library
    {
        set
        {
            if (null != library)
            {
                throw new Exception("Library can only be set once");
            }
    
            this.library = value;
    
            // Set our child objects
            this.ObjTagExplorerControl.Library = library;
            this.ObjAITagExplorerControl.Library = library;
            this.ObjAuthorExplorerControl.Library = library;
            this.ObjPublicationExplorerControl.Library = library;
            this.ObjReadingStageExplorerControl.Library = library;
            this.ObjYearExplorerControl.Library = library;
            this.ObjRatingExplorerControl.Library = library;
            this.ObjThemeExplorerControl.Library = library;
            this.ObjTypeExplorerControl.Library = library;

    That's 9 (nine!) child controllers each going through the entire lib, which @ 29K+ docs is becoming a performance issue -- IFF that 'long slow UI update wait' wasn't coming from elsewhere in the code: once I find out what I b0rked in there these ones are next on the bottleneck list as their <control>.Library SETTER will trigger a control-internal Reset() and a lot more.

  3. TAKE NOTE that these reset+update cycles are pretty premature/kinda useless as the Library updates asynchronously in the background: each of these controls will encounter another number of PDFDocuments in the same library when libraries get large, like mine.

    The nett visible effect of this can be observed easily in the Home tab: quite often you'll notice that the number of documents in the library is too small / incorrect.

    When you then click the graph button that number gets updated to the correct, larger number as that click handler (among a few others) has this UI update logic included under the hood: WebLibraryDetailControl::UpdateLibraryStatistics_Headers(), invoked via void UpdateLibraryStatistics().

    The premature invocation of those GetNodeItems' PDFDocuments-fueled internal loops is harder to observe elsewhere as those controls get refreshed when they become visible as you switch to their view.

  4. Using the keyboard to run through the document list, while the preview pane is updated on the right side, has become very sluggish, neigh unusable. I bet I screwed up somewhere in there in one of my previous commits as this isn't deadlock material but rather more smelling like yet more 'enforced OCR yes/no' crap hitting the ceiling fan and spreading joy 😞 -- I know I've had quite a bit of bother with some of those PDFDocument get/set properties which perform hidden OCR or text file-set collecting activities inline.

    GetOCRText(1) can be found in a few places in the code as an old hack to trigger OCR work on a document; my current guess is I must've been overeager somewhere with using one or more of those PDFDocument property buggers which hide elaborate getter/setters. But... only further investigative work will uncover the actual culprit(s) in the current codebase of mine. 🕵


Well, that's progress for ya. Back to going through the code; this ends this rant, which at least serves as a checklist for me as things get pretty involved now.


[EDIT]

Turns out most of the trouble was caused by UI cruft instead of locks pending forever: s commit SHA-1: 2e9c486986f6309a4718dbceb05a577d10712701 and SHA-1: ea2f8e713d6a91742a65fb04e2472276886749aa.

This goes to show that initial assumptions, though sticky for a while, are no guarantee you'll find reality to match. 🤡

At the time of writing this new state of affairs must still be tested with the large 29K+ library...

GerHobbelt commented 4 years ago

Closing and decluttering the issue list so it stays workable for me: fixed in https://github.com/GerHobbelt/qiqqa-open-source mainline=master branch, i.e. v82pre3 experimental release and beyond.