Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
GerHobbelt opened this issue Aug 23, 2019 · 2 comments
Labels
Projects
Milestone

Comments

@GerHobbelt
Copy link
Collaborator

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
Copy link
Collaborator Author

GerHobbelt commented Aug 26, 2019

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: 2e9c486 and SHA-1: ea2f8e7.

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 added a commit to GerHobbelt/qiqqa-open-source that referenced this issue Aug 26, 2019
… while I dig further to uncover the *slowtard* culprits. Mother Of All Commits. Can be split up into multiple commits if needed later on in a separate branch. Actions done in the past days while hunting the shite:

- `library.GetDocumentByFingerprint(fingerprint)` can get you a **NULL** and quite a few spots in the code didn't account for this; ran into several crashes along the way due to this. Now code has been augmented with Error logging, etc. to at least report these occurrences as they often hint at internal sanity issues of the library or codebase.
- using C# 6.0 language constructs where it helps readability. I'm happy the `?` and `??` operators are around today.
- **IMPORTANT**: instrument the Hell out of those C# `lock(...)` constructs to detect if any of those buggers is deadlocking or at least retarding on me. The code idiom looks like this:

            Utilities.LockPerfTimer l1_clk = Utilities.LockPerfChecker.Start();
            lock (thread_lock)
            {
                l1_clk.LockPerfTimerStop();

   where the `LockPerfChecker.Start()` call sets up an async timer to fire when waiting for the `.LockPerfTimerStop()` takes too much time. Any such occurrence is then logged to Qiqqa.log

- *NUKED* the Chat timer ticker as it hampered the debugging sessions no end and is pretty useless right now anyway.

  TODO: Uncomment that `#if false` in ChatControl when I'm done.

- fixed a NULL string crash in the Window Size Restore logic (StandardWindow.cs)

- added more Debug level logging to monitor the action while the large Library fills up

- `BlackWhiteListManager`: improved error reporting: either the file isn't there *or* it's corrupt and we should be able to identify both situations from the log.

- `FolderWatcher`: rename the ill-named `previous_folder_to_watch` variable.

- `FolderWatcher`: `file_system_watcher.Path = folder_to_watch;` will crash when you happen to have configured a non-existing path to watch. The new code takes proper care of this situation.

- `NotificationManager.Notification`: when the tooltip is NULL, it will use the notification text itself instead. Corrected the invocations of this API to use this feature and reduce the amount of string duplication in the source code.

- use the preferred/MS-advised idiom for `lock(xxx_lock) { ...use xxx...}` where a separate `object` is created for the lock thoughout the codebase: this prevents odd failures with locking Dictionary-types etc. -- I haven't explicitly scanned the codebase to uncover all but most such situations have been corrected.

- `Library:BuildFromDocumentRepository()`: log the progress loading the documents once every X seconds rather than every 250 documents: sometimes loading is fast, sometimes slow and I'd rather observe such in the logfile.

- `Library` performance: `internal HashSet<string> GetAllDocumentFingerprints()` has been recoded and should be (marginally) faster now.

- `Library` `PDFDocuments` performance: several controls used code like `library.PDFDocuments.Count` which is rather costly as it copies an entire set just to measure its size. Using the `library.PDFDocuments_IncludingDeleted_Count` API instead.

- `Library`: `internal HashSet<string> GetDocumentFingerprintsWithKeyword(string keyword)` has been augmented with a `Publication` match check and has been made thread-safe like the rest of them: all code accessing the `pdf_documents` member should be thread-safe or you're in for spurious trouble.

- `LibraryFilterControl`: encoded all sorting mode delegates the same way: as functions in `PDFDocumentListSorters`. (Code consistency)

- As I ran into several crashes in the application init phase, a few *singleton objects*' idiom has changed from

        class WebLibraryManager
        {
          public static WebLibraryManager Instance = new WebLibraryManager();

   to

        class WebLibraryManager
        {
          private static WebLibraryManager __instance = null;
          public static WebLibraryManager Instance
          {
            get
            {
                if (null == __instance)
                {
                    __instance = new WebLibraryManager();
                }
                return __instance;
            }
         }

   which results in a singleton with 'init on first demand' behaviour, hence singleton instances which initialize later during the application init phase, where various internal APIs become safely available. (See also the `Logging` entry below)

- `PDFDocumentCitationManager`: despite a slew of `lock(...)` code, there was still thread-UNSAFE usage in there in the `CloneFrom()` method, due to the lock being **instance**-particular. This has been resolved in the class' code.

- `GoogleBibTexSnifferControl.xaml.cs`: fixed another spurious NULL-induced crash

- `FeatureTracking:GoogleAnalysicsSubmitter` begets a 403 (Forbidden) from Google Analytics. Report such in the logfile instead of letting it pass silently. (Not that I care that much about feature tracking right now, but this was part of the larger code review re HTTP access from Qiqqa)

- `StatusBar`: move the thread-creating/queueing `Dispatcher.BeginInvoke(...)` call outside the critical section: this will prevent potential deadlock scenarios (not sure if this is one of the retarders, but at least it popped up during code review).

- BibTeXAssembler/Parser: append some harmless whitespace at the end to help reduce the number of out-of-bounds exceptions in the parser/lexer while keeping the code simple

- `GUITools::public static void ScrollToTop(ListView listview)` was seriously haranguing the logfile with a slew of deep-level exceptions while large libraries are loading and the UI is set up in Qiqqa, due to the listview not yet being ready to receive such attention. After trying a few things, I *think* I got it fixed by checking the `Height` property for `NaN` to preclude the entire offending code chunk.

   **Interestingly enough, said code chunk will now *never execute* due to the premature UI updating done via those `.Library = library` code lines mentioned in  jimmejardine#55 (comment)

   See the comment chunk above this edit for more info.

- `Logging`: ohhhh boy! It started with working on the crashes detected at early app init time, where there were several `Logging.Info()` calls which would trigger a crash in the `Logger` constructor code, due to other bits of the code not being ready yet. Now the `Logging` class buffers any such 'premature' logging reports in a RAM buffer and dumps them to logfile as soon as it comes available. Meanwhile...

- `Logging`:  **WEIRDING-OUT ALERT**

   ... seems to be a **very odd class** as any methods added to it seem to **automagickally lock** i.e. are thread-safety-protected, but I cannot find out how this was done -- it's been too long since I last went neck-deep into .NET and I seem to recall some behaviour like that from anything log4net-related from back in my days with .NET and log4net. Anyhow, after gnashing my teeth and killing a few debuggers I went for the alternative: the work-around. Those functions have been moved into the new `LockPerfChecker.cs` file/class, which is nice re code organization as well. Nevertheless I believe I should those **weird** lockups I had with lock test code in `LogError` there while I was debugging and testing the new `lock()` instrumentation code. Very odd indeed, unless one assumes the entire `Logging` class is thread-lock-protected in *bulk*. **WEIRDING-OUT ALERT**

- `QueueUserWorkItem`: disable test code in there which is only slowing us down.

- removed unused source files, e.g.

      <Compile Include="Finance\YieldSolverAnnualised.cs" />
      <Compile Include="Finance\YieldSolverContinuous.cs" />
      <Compile Include="Finance\YieldSolver.cs" />
GerHobbelt added a commit to GerHobbelt/qiqqa-open-source that referenced this issue Aug 26, 2019
GerHobbelt added a commit to GerHobbelt/qiqqa-open-source that referenced this issue Aug 26, 2019
…sing the keyboard: it's the repeated invocation of the code line `ThemeTabItem.AddToApplicationResources(app)` for each individual control being instantiated that killed the focus semi-randomly. That code was introduced/activated that way a while ago to better support the MSVS2019 Designer -- which should still work as before.

- fixes several issues reported in jimmejardine#55 (comment) : the keyboard scrolling no longer suffers from long delays -- where I first suspected thread locking it turns out to be a hassle with WPF (grrrrrr); I cannot explain **how exactly** WPF caused the lethargic slowdown, but the fact is that it's gone now. The fact I cannot explain this makes this codebase brittle from my point of view. Alas.

- it *looks* like I can gt away with a bit of a performance boost as that `listview.UpdateLayout();` from `GUITools::ScrollToTop()` looks like it can be safely moved into the IsVisibleChanged handler for said ListView(s)... hmmmmm
GerHobbelt added a commit to GerHobbelt/qiqqa-open-source that referenced this issue Oct 2, 2019
… while I dig further to uncover the *slowtard* culprits. Mother Of All Commits. Can be split up into multiple commits if needed later on in a separate branch. Actions done in the past days while hunting the shite:

- `library.GetDocumentByFingerprint(fingerprint)` can get you a **NULL** and quite a few spots in the code didn't account for this; ran into several crashes along the way due to this. Now code has been augmented with Error logging, etc. to at least report these occurrences as they often hint at internal sanity issues of the library or codebase.
- using C# 6.0 language constructs where it helps readability. I'm happy the `?` and `??` operators are around today.
- **IMPORTANT**: instrument the Hell out of those C# `lock(...)` constructs to detect if any of those buggers is deadlocking or at least retarding on me. The code idiom looks like this:

            Utilities.LockPerfTimer l1_clk = Utilities.LockPerfChecker.Start();
            lock (thread_lock)
            {
                l1_clk.LockPerfTimerStop();

   where the `LockPerfChecker.Start()` call sets up an async timer to fire when waiting for the `.LockPerfTimerStop()` takes too much time. Any such occurrence is then logged to Qiqqa.log

- *NUKED* the Chat timer ticker as it hampered the debugging sessions no end and is pretty useless right now anyway.

  TODO: Uncomment that `#if false` in ChatControl when I'm done.

- fixed a NULL string crash in the Window Size Restore logic (StandardWindow.cs)

- added more Debug level logging to monitor the action while the large Library fills up

- `BlackWhiteListManager`: improved error reporting: either the file isn't there *or* it's corrupt and we should be able to identify both situations from the log.

- `FolderWatcher`: rename the ill-named `previous_folder_to_watch` variable.

- `FolderWatcher`: `file_system_watcher.Path = folder_to_watch;` will crash when you happen to have configured a non-existing path to watch. The new code takes proper care of this situation.

- `NotificationManager.Notification`: when the tooltip is NULL, it will use the notification text itself instead. Corrected the invocations of this API to use this feature and reduce the amount of string duplication in the source code.

- use the preferred/MS-advised idiom for `lock(xxx_lock) { ...use xxx...}` where a separate `object` is created for the lock thoughout the codebase: this prevents odd failures with locking Dictionary-types etc. -- I haven't explicitly scanned the codebase to uncover all but most such situations have been corrected.

- `Library:BuildFromDocumentRepository()`: log the progress loading the documents once every X seconds rather than every 250 documents: sometimes loading is fast, sometimes slow and I'd rather observe such in the logfile.

- `Library` performance: `internal HashSet<string> GetAllDocumentFingerprints()` has been recoded and should be (marginally) faster now.

- `Library` `PDFDocuments` performance: several controls used code like `library.PDFDocuments.Count` which is rather costly as it copies an entire set just to measure its size. Using the `library.PDFDocuments_IncludingDeleted_Count` API instead.

- `Library`: `internal HashSet<string> GetDocumentFingerprintsWithKeyword(string keyword)` has been augmented with a `Publication` match check and has been made thread-safe like the rest of them: all code accessing the `pdf_documents` member should be thread-safe or you're in for spurious trouble.

- `LibraryFilterControl`: encoded all sorting mode delegates the same way: as functions in `PDFDocumentListSorters`. (Code consistency)

- As I ran into several crashes in the application init phase, a few *singleton objects*' idiom has changed from

        class WebLibraryManager
        {
          public static WebLibraryManager Instance = new WebLibraryManager();

   to

        class WebLibraryManager
        {
          private static WebLibraryManager __instance = null;
          public static WebLibraryManager Instance
          {
            get
            {
                if (null == __instance)
                {
                    __instance = new WebLibraryManager();
                }
                return __instance;
            }
         }

   which results in a singleton with 'init on first demand' behaviour, hence singleton instances which initialize later during the application init phase, where various internal APIs become safely available. (See also the `Logging` entry below)

- `PDFDocumentCitationManager`: despite a slew of `lock(...)` code, there was still thread-UNSAFE usage in there in the `CloneFrom()` method, due to the lock being **instance**-particular. This has been resolved in the class' code.

- `GoogleBibTexSnifferControl.xaml.cs`: fixed another spurious NULL-induced crash

- `FeatureTracking:GoogleAnalysicsSubmitter` begets a 403 (Forbidden) from Google Analytics. Report such in the logfile instead of letting it pass silently. (Not that I care that much about feature tracking right now, but this was part of the larger code review re HTTP access from Qiqqa)

- `StatusBar`: move the thread-creating/queueing `Dispatcher.BeginInvoke(...)` call outside the critical section: this will prevent potential deadlock scenarios (not sure if this is one of the retarders, but at least it popped up during code review).

- BibTeXAssembler/Parser: append some harmless whitespace at the end to help reduce the number of out-of-bounds exceptions in the parser/lexer while keeping the code simple

- `GUITools::public static void ScrollToTop(ListView listview)` was seriously haranguing the logfile with a slew of deep-level exceptions while large libraries are loading and the UI is set up in Qiqqa, due to the listview not yet being ready to receive such attention. After trying a few things, I *think* I got it fixed by checking the `Height` property for `NaN` to preclude the entire offending code chunk.

   **Interestingly enough, said code chunk will now *never execute* due to the premature UI updating done via those `.Library = library` code lines mentioned in  jimmejardine#55 (comment)

   See the comment chunk above this edit for more info.

- `Logging`: ohhhh boy! It started with working on the crashes detected at early app init time, where there were several `Logging.Info()` calls which would trigger a crash in the `Logger` constructor code, due to other bits of the code not being ready yet. Now the `Logging` class buffers any such 'premature' logging reports in a RAM buffer and dumps them to logfile as soon as it comes available. Meanwhile...

- `Logging`:  **WEIRDING-OUT ALERT**

   ... seems to be a **very odd class** as any methods added to it seem to **automagickally lock** i.e. are thread-safety-protected, but I cannot find out how this was done -- it's been too long since I last went neck-deep into .NET and I seem to recall some behaviour like that from anything log4net-related from back in my days with .NET and log4net. Anyhow, after gnashing my teeth and killing a few debuggers I went for the alternative: the work-around. Those functions have been moved into the new `LockPerfChecker.cs` file/class, which is nice re code organization as well. Nevertheless I believe I should those **weird** lockups I had with lock test code in `LogError` there while I was debugging and testing the new `lock()` instrumentation code. Very odd indeed, unless one assumes the entire `Logging` class is thread-lock-protected in *bulk*. **WEIRDING-OUT ALERT**

- `QueueUserWorkItem`: disable test code in there which is only slowing us down.

- removed unused source files, e.g.

      <Compile Include="Finance\YieldSolverAnnualised.cs" />
      <Compile Include="Finance\YieldSolverContinuous.cs" />
      <Compile Include="Finance\YieldSolver.cs" />
GerHobbelt added a commit to GerHobbelt/qiqqa-open-source that referenced this issue Oct 2, 2019
GerHobbelt added a commit to GerHobbelt/qiqqa-open-source that referenced this issue Oct 2, 2019
…sing the keyboard: it's the repeated invocation of the code line `ThemeTabItem.AddToApplicationResources(app)` for each individual control being instantiated that killed the focus semi-randomly. That code was introduced/activated that way a while ago to better support the MSVS2019 Designer -- which should still work as before.

- fixes several issues reported in jimmejardine#55 (comment) : the keyboard scrolling no longer suffers from long delays -- where I first suspected thread locking it turns out to be a hassle with WPF (grrrrrr); I cannot explain **how exactly** WPF caused the lethargic slowdown, but the fact is that it's gone now. The fact I cannot explain this makes this codebase brittle from my point of view. Alas.

- it *looks* like I can gt away with a bit of a performance boost as that `listview.UpdateLayout();` from `GUITools::ScrollToTop()` looks like it can be safely moved into the IsVisibleChanged handler for said ListView(s)... hmmmmm
GerHobbelt added a commit to GerHobbelt/qiqqa-open-source that referenced this issue Oct 3, 2019
… while I dig further to uncover the *slowtard* culprits. Mother Of All Commits. Can be split up into multiple commits if needed later on in a separate branch. Actions done in the past days while hunting the shite:

- `library.GetDocumentByFingerprint(fingerprint)` can get you a **NULL** and quite a few spots in the code didn't account for this; ran into several crashes along the way due to this. Now code has been augmented with Error logging, etc. to at least report these occurrences as they often hint at internal sanity issues of the library or codebase.
- using C# 6.0 language constructs where it helps readability. I'm happy the `?` and `??` operators are around today.
- **IMPORTANT**: instrument the Hell out of those C# `lock(...)` constructs to detect if any of those buggers is deadlocking or at least retarding on me. The code idiom looks like this:

            Utilities.LockPerfTimer l1_clk = Utilities.LockPerfChecker.Start();
            lock (thread_lock)
            {
                l1_clk.LockPerfTimerStop();

   where the `LockPerfChecker.Start()` call sets up an async timer to fire when waiting for the `.LockPerfTimerStop()` takes too much time. Any such occurrence is then logged to Qiqqa.log

- *NUKED* the Chat timer ticker as it hampered the debugging sessions no end and is pretty useless right now anyway.

  TODO: Uncomment that `#if false` in ChatControl when I'm done.

- fixed a NULL string crash in the Window Size Restore logic (StandardWindow.cs)

- added more Debug level logging to monitor the action while the large Library fills up

- `BlackWhiteListManager`: improved error reporting: either the file isn't there *or* it's corrupt and we should be able to identify both situations from the log.

- `FolderWatcher`: rename the ill-named `previous_folder_to_watch` variable.

- `FolderWatcher`: `file_system_watcher.Path = folder_to_watch;` will crash when you happen to have configured a non-existing path to watch. The new code takes proper care of this situation.

- `NotificationManager.Notification`: when the tooltip is NULL, it will use the notification text itself instead. Corrected the invocations of this API to use this feature and reduce the amount of string duplication in the source code.

- use the preferred/MS-advised idiom for `lock(xxx_lock) { ...use xxx...}` where a separate `object` is created for the lock thoughout the codebase: this prevents odd failures with locking Dictionary-types etc. -- I haven't explicitly scanned the codebase to uncover all but most such situations have been corrected.

- `Library:BuildFromDocumentRepository()`: log the progress loading the documents once every X seconds rather than every 250 documents: sometimes loading is fast, sometimes slow and I'd rather observe such in the logfile.

- `Library` performance: `internal HashSet<string> GetAllDocumentFingerprints()` has been recoded and should be (marginally) faster now.

- `Library` `PDFDocuments` performance: several controls used code like `library.PDFDocuments.Count` which is rather costly as it copies an entire set just to measure its size. Using the `library.PDFDocuments_IncludingDeleted_Count` API instead.

- `Library`: `internal HashSet<string> GetDocumentFingerprintsWithKeyword(string keyword)` has been augmented with a `Publication` match check and has been made thread-safe like the rest of them: all code accessing the `pdf_documents` member should be thread-safe or you're in for spurious trouble.

- `LibraryFilterControl`: encoded all sorting mode delegates the same way: as functions in `PDFDocumentListSorters`. (Code consistency)

- As I ran into several crashes in the application init phase, a few *singleton objects*' idiom has changed from

        class WebLibraryManager
        {
          public static WebLibraryManager Instance = new WebLibraryManager();

   to

        class WebLibraryManager
        {
          private static WebLibraryManager __instance = null;
          public static WebLibraryManager Instance
          {
            get
            {
                if (null == __instance)
                {
                    __instance = new WebLibraryManager();
                }
                return __instance;
            }
         }

   which results in a singleton with 'init on first demand' behaviour, hence singleton instances which initialize later during the application init phase, where various internal APIs become safely available. (See also the `Logging` entry below)

- `PDFDocumentCitationManager`: despite a slew of `lock(...)` code, there was still thread-UNSAFE usage in there in the `CloneFrom()` method, due to the lock being **instance**-particular. This has been resolved in the class' code.

- `GoogleBibTexSnifferControl.xaml.cs`: fixed another spurious NULL-induced crash

- `FeatureTracking:GoogleAnalysicsSubmitter` begets a 403 (Forbidden) from Google Analytics. Report such in the logfile instead of letting it pass silently. (Not that I care that much about feature tracking right now, but this was part of the larger code review re HTTP access from Qiqqa)

- `StatusBar`: move the thread-creating/queueing `Dispatcher.BeginInvoke(...)` call outside the critical section: this will prevent potential deadlock scenarios (not sure if this is one of the retarders, but at least it popped up during code review).

- BibTeXAssembler/Parser: append some harmless whitespace at the end to help reduce the number of out-of-bounds exceptions in the parser/lexer while keeping the code simple

- `GUITools::public static void ScrollToTop(ListView listview)` was seriously haranguing the logfile with a slew of deep-level exceptions while large libraries are loading and the UI is set up in Qiqqa, due to the listview not yet being ready to receive such attention. After trying a few things, I *think* I got it fixed by checking the `Height` property for `NaN` to preclude the entire offending code chunk.

   **Interestingly enough, said code chunk will now *never execute* due to the premature UI updating done via those `.Library = library` code lines mentioned in  jimmejardine#55 (comment)

   See the comment chunk above this edit for more info.

- `Logging`: ohhhh boy! It started with working on the crashes detected at early app init time, where there were several `Logging.Info()` calls which would trigger a crash in the `Logger` constructor code, due to other bits of the code not being ready yet. Now the `Logging` class buffers any such 'premature' logging reports in a RAM buffer and dumps them to logfile as soon as it comes available. Meanwhile...

- `Logging`:  **WEIRDING-OUT ALERT**

   ... seems to be a **very odd class** as any methods added to it seem to **automagickally lock** i.e. are thread-safety-protected, but I cannot find out how this was done -- it's been too long since I last went neck-deep into .NET and I seem to recall some behaviour like that from anything log4net-related from back in my days with .NET and log4net. Anyhow, after gnashing my teeth and killing a few debuggers I went for the alternative: the work-around. Those functions have been moved into the new `LockPerfChecker.cs` file/class, which is nice re code organization as well. Nevertheless I believe I should those **weird** lockups I had with lock test code in `LogError` there while I was debugging and testing the new `lock()` instrumentation code. Very odd indeed, unless one assumes the entire `Logging` class is thread-lock-protected in *bulk*. **WEIRDING-OUT ALERT**

- `QueueUserWorkItem`: disable test code in there which is only slowing us down.

- removed unused source files, e.g.

      <Compile Include="Finance\YieldSolverAnnualised.cs" />
      <Compile Include="Finance\YieldSolverContinuous.cs" />
      <Compile Include="Finance\YieldSolver.cs" />
GerHobbelt added a commit to GerHobbelt/qiqqa-open-source that referenced this issue Oct 3, 2019
GerHobbelt added a commit to GerHobbelt/qiqqa-open-source that referenced this issue Oct 3, 2019
…sing the keyboard: it's the repeated invocation of the code line `ThemeTabItem.AddToApplicationResources(app)` for each individual control being instantiated that killed the focus semi-randomly. That code was introduced/activated that way a while ago to better support the MSVS2019 Designer -- which should still work as before.

- fixes several issues reported in jimmejardine#55 (comment) : the keyboard scrolling no longer suffers from long delays -- where I first suspected thread locking it turns out to be a hassle with WPF (grrrrrr); I cannot explain **how exactly** WPF caused the lethargic slowdown, but the fact is that it's gone now. The fact I cannot explain this makes this codebase brittle from my point of view. Alas.

- it *looks* like I can gt away with a bit of a performance boost as that `listview.UpdateLayout();` from `GUITools::ScrollToTop()` looks like it can be safely moved into the IsVisibleChanged handler for said ListView(s)... hmmmmm
@GerHobbelt GerHobbelt added the 🦸‍♀️enhancement🦸‍♂️ New feature or request label Oct 4, 2019
@GerHobbelt GerHobbelt added this to the Our Glorious Future milestone Oct 9, 2019
@GerHobbelt
Copy link
Collaborator Author

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.

@GerHobbelt GerHobbelt modified the milestones: Our Glorious Future, v82 Nov 5, 2019
@GerHobbelt GerHobbelt added this to Done in v82pre4 Nov 5, 2019
GerHobbelt added a commit that referenced this issue Nov 5, 2019
…ig further to uncover the *slowtard* culprits. Mother Of All Commits. Can be split up into multiple commits if needed later on in a separate branch. Actions done in the past days while hunting the shite:

- `library.GetDocumentByFingerprint(fingerprint)` can get you a **NULL** and quite a few spots in the code didn't account for this; ran into several crashes along the way due to this. Now code has been augmented with Error logging, etc. to at least report these occurrences as they often hint at internal sanity issues of the library or codebase.
- using C# 6.0 language constructs where it helps readability. I'm happy the `?` and `??` operators are around today.
- **IMPORTANT**: instrument the Hell out of those C# `lock(...)` constructs to detect if any of those buggers is deadlocking or at least retarding on me. The code idiom looks like this:

            Utilities.LockPerfTimer l1_clk = Utilities.LockPerfChecker.Start();
            lock (thread_lock)
            {
                l1_clk.LockPerfTimerStop();

   where the `LockPerfChecker.Start()` call sets up an async timer to fire when waiting for the `.LockPerfTimerStop()` takes too much time. Any such occurrence is then logged to Qiqqa.log

- *NUKED* the Chat timer ticker as it hampered the debugging sessions no end and is pretty useless right now anyway.

  TODO: Uncomment that `#if false` in ChatControl when I'm done.

- fixed a NULL string crash in the Window Size Restore logic (StandardWindow.cs)

- added more Debug level logging to monitor the action while the large Library fills up

- `BlackWhiteListManager`: improved error reporting: either the file isn't there *or* it's corrupt and we should be able to identify both situations from the log.

- `FolderWatcher`: rename the ill-named `previous_folder_to_watch` variable.

- `FolderWatcher`: `file_system_watcher.Path = folder_to_watch;` will crash when you happen to have configured a non-existing path to watch. The new code takes proper care of this situation.

- `NotificationManager.Notification`: when the tooltip is NULL, it will use the notification text itself instead. Corrected the invocations of this API to use this feature and reduce the amount of string duplication in the source code.

- use the preferred/MS-advised idiom for `lock(xxx_lock) { ...use xxx...}` where a separate `object` is created for the lock thoughout the codebase: this prevents odd failures with locking Dictionary-types etc. -- I haven't explicitly scanned the codebase to uncover all but most such situations have been corrected.

- `Library:BuildFromDocumentRepository()`: log the progress loading the documents once every X seconds rather than every 250 documents: sometimes loading is fast, sometimes slow and I'd rather observe such in the logfile.

- `Library` performance: `internal HashSet<string> GetAllDocumentFingerprints()` has been recoded and should be (marginally) faster now.

- `Library` `PDFDocuments` performance: several controls used code like `library.PDFDocuments.Count` which is rather costly as it copies an entire set just to measure its size. Using the `library.PDFDocuments_IncludingDeleted_Count` API instead.

- `Library`: `internal HashSet<string> GetDocumentFingerprintsWithKeyword(string keyword)` has been augmented with a `Publication` match check and has been made thread-safe like the rest of them: all code accessing the `pdf_documents` member should be thread-safe or you're in for spurious trouble.

- `LibraryFilterControl`: encoded all sorting mode delegates the same way: as functions in `PDFDocumentListSorters`. (Code consistency)

- As I ran into several crashes in the application init phase, a few *singleton objects*' idiom has changed from

        class WebLibraryManager
        {
          public static WebLibraryManager Instance = new WebLibraryManager();

   to

        class WebLibraryManager
        {
          private static WebLibraryManager __instance = null;
          public static WebLibraryManager Instance
          {
            get
            {
                if (null == __instance)
                {
                    __instance = new WebLibraryManager();
                }
                return __instance;
            }
         }

   which results in a singleton with 'init on first demand' behaviour, hence singleton instances which initialize later during the application init phase, where various internal APIs become safely available. (See also the `Logging` entry below)

- `PDFDocumentCitationManager`: despite a slew of `lock(...)` code, there was still thread-UNSAFE usage in there in the `CloneFrom()` method, due to the lock being **instance**-particular. This has been resolved in the class' code.

- `GoogleBibTexSnifferControl.xaml.cs`: fixed another spurious NULL-induced crash

- `FeatureTracking:GoogleAnalysicsSubmitter` begets a 403 (Forbidden) from Google Analytics. Report such in the logfile instead of letting it pass silently. (Not that I care that much about feature tracking right now, but this was part of the larger code review re HTTP access from Qiqqa)

- `StatusBar`: move the thread-creating/queueing `Dispatcher.BeginInvoke(...)` call outside the critical section: this will prevent potential deadlock scenarios (not sure if this is one of the retarders, but at least it popped up during code review).

- BibTeXAssembler/Parser: append some harmless whitespace at the end to help reduce the number of out-of-bounds exceptions in the parser/lexer while keeping the code simple

- `GUITools::public static void ScrollToTop(ListView listview)` was seriously haranguing the logfile with a slew of deep-level exceptions while large libraries are loading and the UI is set up in Qiqqa, due to the listview not yet being ready to receive such attention. After trying a few things, I *think* I got it fixed by checking the `Height` property for `NaN` to preclude the entire offending code chunk.

   **Interestingly enough, said code chunk will now *never execute* due to the premature UI updating done via those `.Library = library` code lines mentioned in  #55 (comment)

   See the comment chunk above this edit for more info.

- `Logging`: ohhhh boy! It started with working on the crashes detected at early app init time, where there were several `Logging.Info()` calls which would trigger a crash in the `Logger` constructor code, due to other bits of the code not being ready yet. Now the `Logging` class buffers any such 'premature' logging reports in a RAM buffer and dumps them to logfile as soon as it comes available. Meanwhile...

- `Logging`:  **WEIRDING-OUT ALERT**

   ... seems to be a **very odd class** as any methods added to it seem to **automagickally lock** i.e. are thread-safety-protected, but I cannot find out how this was done -- it's been too long since I last went neck-deep into .NET and I seem to recall some behaviour like that from anything log4net-related from back in my days with .NET and log4net. Anyhow, after gnashing my teeth and killing a few debuggers I went for the alternative: the work-around. Those functions have been moved into the new `LockPerfChecker.cs` file/class, which is nice re code organization as well. Nevertheless I believe I should those **weird** lockups I had with lock test code in `LogError` there while I was debugging and testing the new `lock()` instrumentation code. Very odd indeed, unless one assumes the entire `Logging` class is thread-lock-protected in *bulk*. **WEIRDING-OUT ALERT**

- `QueueUserWorkItem`: disable test code in there which is only slowing us down.

- removed unused source files, e.g.

      <Compile Include="Finance\YieldSolverAnnualised.cs" />
      <Compile Include="Finance\YieldSolverContinuous.cs" />
      <Compile Include="Finance\YieldSolver.cs" />
GerHobbelt added a commit that referenced this issue Nov 5, 2019
GerHobbelt added a commit that referenced this issue Nov 5, 2019
…sing the keyboard: it's the repeated invocation of the code line `ThemeTabItem.AddToApplicationResources(app)` for each individual control being instantiated that killed the focus semi-randomly. That code was introduced/activated that way a while ago to better support the MSVS2019 Designer -- which should still work as before.

- fixes several issues reported in #55 (comment) : the keyboard scrolling no longer suffers from long delays -- where I first suspected thread locking it turns out to be a hassle with WPF (grrrrrr); I cannot explain **how exactly** WPF caused the lethargic slowdown, but the fact is that it's gone now. The fact I cannot explain this makes this codebase brittle from my point of view. Alas.

- it *looks* like I can gt away with a bit of a performance boost as that `listview.UpdateLayout();` from `GUITools::ScrollToTop()` looks like it can be safely moved into the IsVisibleChanged handler for said ListView(s)... hmmmmm
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
No open projects
v82pre4
  
Done
Development

No branches or pull requests

1 participant