Introduce a Cache class
ClosedPublic

Authored by ervin on Apr 9 2017, 7:51 AM.

Details

Summary

This should help caching the data we tend to throw away out of the
Storage. This will speed up our tree generation and also allow to
provide a better service to features like contexts workday.

This is not connected to anything yet, will come in further commits.

Diff Detail

Repository
R4 Zanshin
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
ervin created this revision.Apr 9 2017, 7:51 AM
franckarrecot edited edge metadata.Apr 9 2017, 10:03 AM

Most of your tests are using are plain tags, you tried out once the "context" tag, even though on some tests case you would use two tags (so one could be a context), I guess that's fine because they are implemented the same way ?

franckarrecot accepted this revision.Apr 9 2017, 10:15 AM

+1 from me, would be nice to have a second validation from Mario since it's a long patch :-)

src/akonadi/akonadicache.cpp
249

both on TagAdded and onTagChanged have the same code, it could be factored out.

This revision is now accepted and ready to land.Apr 9 2017, 10:15 AM
ervin updated this revision to Diff 14469.May 13 2017, 4:02 PM

Ready for review now

dfaure requested changes to this revision.May 14 2017, 10:02 PM
dfaure added inline comments.
src/akonadi/akonadicache.cpp
73

I'm curious, why not a lambda?

121

This list gets detached below (due to modifying a copy which is then set back into the container).
Wouldn't it be faster to use [] here in order to get a ref, and append "in place" in the container?

operator[] calls detach() of course, but won't actually detach since no copying happened, in that solution.

128

... and this line can then go away, making the code shorter and faster.

179

same here

182

you don't use { ... } for one-line bodies in the rest of the file.

231

Iterating over keys() is slow, it requires creating and filling a temporary container.
Use iterators.

249

my reaction as well.

269

if this used an iterator, it wouldn't need to do a double lookup (once for contains, once for operator [])

274

if this used an iterator, it wouldn't need to do a double lookup (once for contains, once for operator [])

292

double lookup (also looks a bit like code duplication)

320

slow, use iterator

322

same

This revision now requires changes to proceed.May 14 2017, 10:02 PM
ervin marked 12 inline comments as done.May 15 2017, 2:44 PM
ervin added inline comments.
src/akonadi/akonadicache.cpp
73

Because I prefer to use higher level constructs whenever I can. "Lambda vs mem_fn/bind" is the "raw for loop vs algorithms" of the functional constructs. :-)

ervin updated this revision to Diff 14550.May 15 2017, 2:48 PM
ervin edited edge metadata.

Address dfaure's comments

bensi accepted this revision.May 16 2017, 8:20 AM
dfaure added inline comments.May 21 2017, 12:20 PM
src/akonadi/akonadicache.cpp
73

Do you have the book "Effective Modern C++" by Scott Meyers?
Item 34 says "Prefer lambdas to std::bind".
The reasons are

  • more readable (in general, and especially when putting the result into an auto variable)
  • less of a source of error (e.g. if one argument is a function call it will be called when the lambda is called, rather than immediately at the time of bind).
  • more expressive (because you can define the type of capture, while std::bind always copies its arguments - which means you can use std::ref() to "capture" by ref, but that's a hidden gem compared to the more explicit lambda capture syntax)
  • in some cases, lambdas are more efficient (can be inlined at calling site)

std::bind is better only if a move capture is needed, and you can't use C++14,
or with a polymorphic functor (which can be a lambda with an auto parameter if you can use C++14).

You're free to disagree with Scott Meyers though, in a case like here where none of the above arguments actually apply (other than readability, arguably). I just wanted to relay that piece of advice which I found useful and easy to memorize ("prefer lambdas to bind" is certainly easier to "it's complicated, sometimes both work, sometimes lambdas are better") :-)

I got it recently but didn't have time to read it yet. Should be my read in the queue in fact. I admit I sometimes have a love/hate relationship with bind... I'll mull it over. Makes me think half his points apply to one of std::thread ctors too. Thanks for the extra feedback.

ervin updated this revision to Diff 14989.May 30 2017, 4:35 PM

Addressing dfaure's comments

dfaure accepted this revision.May 31 2017, 7:51 AM
This revision is now accepted and ready to land.May 31 2017, 7:51 AM
This revision was automatically updated to reflect the committed changes.