Re: [lmi] PATCH: optimization of view updates in MvcController

[Greg Chicares]

On 2019-06-06 20:55, Vadim Zeitlin wrote:
> 
>  Recent (private) discussions about MvcController reminded me that I never
> asked you to apply the simple patch from
> 
>       https://github.com/vadz/lmi/pull/105

Pulled and tested; I'll push it soon.

> which results in big speedup of ConditionallyEnableItems() which is called
> from every wxEVT_UPDATE_UI event and so is quite performance-critical. How
> big exactly? Benchmarks show that average time of its execution goes down
> from 13.1ms to 0.19ms after the first commit, adding the cache for the
> items of wxItemContainers, and further down to 0.08ms after the second
> commit optimizing the unnecessary calls to SetStringSelection(), i.e. a
> ~160x speedup which is really not bad for such a relatively simple patch.
> If you'd like to redo the benchmarks, you can use

Instead of just repeating the benchmarks you tested, I thought it might
be more useful to try some of my own.

I think what users may notice, even if only subliminally, is the time it
takes to change from one dialog tab to another, which I measured with the
patch below[0]. I checked "Enable supplemental report" and observed how
long it took to switch to and from that page with Ctrl-PgUp and Ctrl-PgDn.
The timing is coarse, and varies greatly, but overall the patch decreased
this page-switching time from about forty msec to about thirty-five. The
"Reports" tab has the greatest number of comboboxes; I saw similar results
with the "Names" tab (which has fewer comboboxes, but many items in each).

I also timed 'gui_test.sh'. Median of three runs:
  ./gui_test.sh  30.49s user 9.72s system 79% cpu 50.478 total [without patch]
  ./gui_test.sh  28.61s user 9.78s system 79% cpu 48.454 total [with patch]
for a four percent improvement. That statistic might not represent the
typical user experience well, because the tested operations aren't chosen
to typify any actual workflow.

Of course, I'm using 'wine', and results for native msw would differ. And
these are only coarse measurements that vary considerably between runs.
I tend to doubt the four percent improvement in my last test, because the
GUI test does so many time-consuming things (like running illustrations and
generating PDF files) which I'd suppose would dominated the total time.

But all of my measurements, however coarse, show this to be a nice incremental
improvement.

> https://github.com/thesiv/lmi/commit/a17d96148ed280d3556e15ac8cb8f086451c9f19
> 
> (linked from the PR description). Other than that I don't really have any
> comments about the changes, I hope they should be self-explanatory, but
> please let me know if you have any questions about them, of course.
> 
>  This PR is not really critical, but it should result in user-visible
> improvement in behaviour and, even though there are no unit tests covering
> this code, I'm relatively confident that it can't break anything. It would
> also be better to apply it before doing any other changes in this code, in
> case we decide to change it as discussed elsewhere, to avoid the chance of
> getting conflicts later.

After reading that, I figured this would be the best way to start working
on backlogged patches and discussions from last week.

I do have a couple questions:

Should this conditional:
        auto const& selected_string = datum->str(datum->ordinal());
        if(itembox->GetStringSelection() != selected_string)
            {
            itembox->SetStringSelection(selected_string);
            }
be handled in wxWidgets? I.e., given
  control->SetStringSelection(s)
should SetStringSelection() automatically compare its argument to
whatever GetStringSelection() would return, and do nothing if they're
identical? (Maybe that depends on whether it's possible and reasonable
to cache the result of GetStringSelection().)

Shouldn't it be possible to use standard containers instead of wxArrayString[1]
as in the second patch below? That fails with:

no matching function for call to 
???wxItemContainer::Set(std::vector<std::__cxx11::basic_string<char> >&)???
             itembox->Set(cached_items);

/opt/lmi/local/include/wx-3.1/wx/ctrlsub.h:289:10:
note:   no known conversion for argument 1 from 
???std::vector<std::__cxx11::basic_string<char> >??? to ???const 
std::vector<wxString, std::allocator<wxString> >&???

---------

[0] "measured with the patch below":

---------8<--------8<--------8<--------8<--------8<--------8<--------8<-------
diff --git a/mvc_controller.cpp b/mvc_controller.cpp
index 77eb54be..e02fc044 100644
--- a/mvc_controller.cpp
+++ b/mvc_controller.cpp
@@ -34,6 +34,7 @@
 #include "mvc_model.hpp"
 #include "mvc_view.hpp"
 #include "rtti_lmi.hpp"
+#include "timer.hpp"
 #include "tn_range.hpp"
 #include "transferor.hpp"
 #include "value_cast.hpp"
@@ -741,6 +742,7 @@ void MvcController::UponInitDialog(wxInitDialogEvent& event)
 
 void MvcController::UponPageChanged(wxBookCtrlBaseEvent& event)
 {
+    Timer timer;
     event.Skip();
 
     int const z = event.GetSelection();
@@ -748,6 +750,7 @@ void MvcController::UponPageChanged(wxBookCtrlBaseEvent& 
event)
     last_selected_page[view_.ResourceFileName()] = z;
 
     ConditionallyEnable();
+    status() << "Update: " << timer.stop().elapsed_msec_str() << std::flush;
 }
 
 /// Veto a page change if Validate() fails--but never veto the very
--------->8-------->8-------->8-------->8-------->8-------->8-------->8-------

[1] "standard containers instead of wxArrayString":

---------8<--------8<--------8<--------8<--------8<--------8<--------8<-------
diff --git a/mvc_controller.cpp b/mvc_controller.cpp
index 03497cb0..7bff5b77 100644
--- a/mvc_controller.cpp
+++ b/mvc_controller.cpp
@@ -334,7 +334,7 @@ void MvcController::ConditionallyEnableItems
         {
         auto& cached_items = itemboxes_cache_[name];
 
-        std::vector<wxString> items;
+        std::vector<std::string> items;
         items.reserve(datum->cardinality());
 
         for(int j = 0; j < datum->cardinality(); ++j)
diff --git a/mvc_controller.hpp b/mvc_controller.hpp
index 57c5815d..1011f008 100644
--- a/mvc_controller.hpp
+++ b/mvc_controller.hpp
@@ -467,7 +467,7 @@ class MvcController final
     MvcModel& model_;
     MvcView const& view_;
 
-    std::unordered_map<std::string, std::vector<wxString>> itemboxes_cache_;
+    std::unordered_map<std::string, std::vector<std::string>> itemboxes_cache_;
 
     wxWindow* last_focused_window_;
 
--------->8-------->8-------->8-------->8-------->8-------->8-------->8-------