lmi
[Top][All Lists]
Advanced

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

[lmi] Updated PRs for column-related optimizations in the census view


From: Vadim Zeitlin
Subject: [lmi] Updated PRs for column-related optimizations in the census view
Date: Wed, 5 Jun 2019 01:08:44 +0200

 Hello,

 We've updated the old (and now closed) PR 18 which significantly optimized
updating columns in the census view, notably switching between fixed and
variable widths for them by rebasing it on the current master and using
C++11 features which were not available when the changes were made
originally, notably move semantics, which really helps a lot.

 The result consists of 2 independent parts (which should hopefully make it
simpler to review and apply them). The first one is

        https://github.com/vadz/lmi/pull/113

and contains changes to CensusView only. The commit message tries to
explain the reason for these changes and the code is hopefully clear
enough, especially when ignoring whitespace changes, i.e. with "-w"
git-diff option or at https://github.com/vadz/lmi/pull/113/files?w=1
I'm not sure how much will you like the use of std::equal() with a lambda
as a predicate. Personally I'm not a huge fan and would have probably
written this using an index-based for loop, but I didn't want to ask Ilya
to change this just in case you did like the current version and doing this
would be counter-productive.

 This change on its own results in almost 10 times speed up when switching
to using variable width-columns, i.e. makes the corresponding menu/toolbar
command almost usable even with large censuses (in our tests the time of
processing this command went down from awfully long 3.5s to only somewhat
annoying 0.36s). However it doesn't help that much with switching to fixed
width columns: my original commit message spoke of 2x speedup, but latest
benchmarking by Ilya shows only ~1.4x improvement, which is still nice to
have but not really life-changing.


 However there is also the second part at

        https://github.com/vadz/lmi/pull/114

As mentioned before, it's independent of the first one, i.e. you could
apply just one of these PRs and not the other one, but ideal would, of
course, be to apply both of them. This one is a bit less trivial, but
hopefully the changes are still clear enough, especially if you view them
commit by commit and not all at once. And it results in ~7x speedup for
both fixed- and variable-width cases (meaning that we get a pretty
impressive ~70x speedup for the latter case when the 2 PRs are combined),
so it's not just a minor improvement. Also, the main idea of this PR is
still a pretty simple one: searching for the member by name is a relatively
time-consuming operation (even when using binary search), so we add a
possibility to do it only once and then access this member by index
directly.


 Please let me know if you have any questions about these PRs and it would
be great if they could finally be applied (the original commit of the PR
113, which started it all, is from 2014...).

 Thanks in advance,
VZ

Attachment: pgpLhoXD9HSi_.pgp
Description: PGP signature


reply via email to

[Prev in Thread] Current Thread [Next in Thread]