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

[Vadim Zeitlin]

On Thu, 13 Jun 2019 18:08:49 +0000 Greg Chicares <address@hidden> wrote:

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

 Thanks!

GC> > which results in big speedup of ConditionallyEnableItems() which is called
GC> > from every wxEVT_UPDATE_UI event and so is quite performance-critical. How
GC> > big exactly? Benchmarks show that average time of its execution goes down
GC> > from 13.1ms to 0.19ms after the first commit, adding the cache for the
GC> > items of wxItemContainers, and further down to 0.08ms after the second
GC> > commit optimizing the unnecessary calls to SetStringSelection(), i.e. a
GC> > ~160x speedup which is really not bad for such a relatively simple patch.
GC> > If you'd like to redo the benchmarks, you can use
GC> 
GC> Instead of just repeating the benchmarks you tested, I thought it might
GC> be more useful to try some of my own.
GC> 
GC> I think what users may notice, even if only subliminally, is the time it
GC> takes to change from one dialog tab to another, which I measured with the
GC> patch below[0]. I checked "Enable supplemental report" and observed how
GC> long it took to switch to and from that page with Ctrl-PgUp and Ctrl-PgDn.
GC> The timing is coarse, and varies greatly, but overall the patch decreased
GC> this page-switching time from about forty msec to about thirty-five.

 This made me suspicious as even though this measurement (which, BTW, I
agree is more representable of the actual improvement experienced by the
user) shouldn't show as big gains as the ones I had given, because we
measured the time taken by just ConditionallyEnableItems() and not the
entire UponPageChanged(), I'd still expect more. So I've asked Ilya to
benchmark the time of the page change using your patch under the actual
native MSW system and the results are indeed quite different from Wine:

GC> The "Reports" tab has the greatest number of comboboxes; I saw similar
GC> results with the "Names" tab (which has fewer comboboxes, but many
GC> items in each).

 Switching to the "Reports" tab took 150ms before and 2-3ms now. For the
"Names" tab, the time went down from 60ms to 2ms and for the "Plan" tab, it
went down from 30ms to 3ms. For all the other pages the changes are inside
the measurement error, but it's still worth mentioning that the time didn't
increase for any of them.

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

 Yes, and the test also does many, many other things, so I didn't bother
with measuring this.

GC> Of course, I'm using 'wine', and results for native msw would differ.

 Indeed, and they're much more significant. Of course, even 150ms is hardly
noticeable (although it is, a little), but it could be more on slower
machines, meaning that it would be not only noticeable but potentially
annoying, while after this change it certainly won't be apparent to the
users unless they're running lmi on their toasters.

GC> I do have a couple questions:
GC> 
GC> Should this conditional:
GC>         auto const& selected_string = datum->str(datum->ordinal());
GC>         if(itembox->GetStringSelection() != selected_string)
GC>             {
GC>             itembox->SetStringSelection(selected_string);
GC>             }
GC> be handled in wxWidgets?

 I did think about this, but it's a bit more complicated because for weird
historical reasons SetStringSelection() is implemented independently in
wxListBox and it's not immediately clear how to make this change without
duplicating the check. I might do this later, but even if such check is
added to SetStringSelection() in wx, this shouldn't pessimize lmi much, as
the real benefit comes from the case when the test is false, i.e. we don't
need to change the selection at all, and nothing will change in this case.

GC> Shouldn't it be possible to use standard containers instead of 
wxArrayString[1]
GC> as in the second patch below?
[for the reference, here is the relevant part of the patch below:]
GC> -    std::unordered_map<std::string, std::vector<wxString>> 
itemboxes_cache_;
GC> +    std::unordered_map<std::string, std::vector<std::string>> 
itemboxes_cache_;

 Sorry, I don't quite understand this: we do already use std::vector<> and
not wxArrayString. What the patch does is replacing wxString with
std::string and this indeed doesn't work because wxItemContainer::Set()
only has an overload taking vector<wxString> and not vector<std::string>.
And while such an overload could, in principle, be added, it might be not
the greatest idea because there is no lossless implicit conversion from
std::string to wxString in general (i.e. for non-pure-ASCII strings). We
could add an overload taking vector<std::wstring>, but this wouldn't help
lmi and I'm also not certain if it's a good idea to hide the fact that this
overload would need to allocate a temporary vector (or array) of wxStrings,
unlike the other overloads which use the provided argument directly,
without any extra allocations.

 So on balance I don't believe there is much to be done here, you can't
avoid using wxStrings in wx GUI code and, in fact, I often intentionally
use wxString even for things for which std::[w]string could be used in such
code, while forbidding any appearance of wxString in non-GUI code. IMHO
this helps with separating GUI code from the rest of the application.

 Regards,
VZ

pgphvqQ5MENo_.pgp
Description: PGP signature