DM-13854: add general-purpose Cache class #50

PaulPrice · 2018-03-23T19:03:34Z

No description provided.

TallJimbo

Looks good. I've got a few miscellaneous comments.

In the class documentation I've asked you to write, please also note that:

Value and Key must be Copyable.
This header should generally only be included in source files, not other headers, because we really don't want all of that multi_index_container stuff being included in source files where it isn't needed. Since this is a class that will be frequently used as a data member, that in turn means that it should usually be held via unique_ptr so it can be forward-declared in the header file.

And to make that last bit of advice easier for users to follow, it might be good to make a CacheFwd.h header file that does that forward-declaration for them. Note that the forward declaration needs to include the template parameter defaults, and then you'll need to remove them from Cache.h and have it include CacheFwd.h.

TallJimbo · 2018-03-23T19:38:37Z

include/lsst/utils/Cache.h

+namespace utils {
+
+/// Cache of most recently used values
+template <typename Key, typename Value, typename KeyHash=boost::hash<Key>,


C++11 has a std::hash that should be preferred.

TallJimbo · 2018-03-23T19:39:54Z

include/lsst/utils/Cache.h

+#include <boost/multi_index/composite_key.hpp>
+#include <boost/multi_index/member.hpp>
+#include <boost/format.hpp>
+#include <boost/functional/hash.hpp>  // boost::hash


LSST convention is to use "" instead of <> for all but system headers.

TallJimbo · 2018-03-23T19:41:29Z

include/lsst/utils/Cache.h

+namespace lsst {
+namespace utils {
+
+/// Cache of most recently used values


Would like to see a bit more in the way of class docs than this.

I believe we've also standardized on using /** ... */ for all but single-line docstrings (https://developer.lsst.io/cpp/api-docs.html#documentation-must-be-delimited-in-javadoc-style).

TallJimbo · 2018-03-23T19:43:38Z

include/lsst/utils/Cache.h

+    /// Dtor
+#ifdef LSST_CACHE_DEBUG
+    ///
+    /// Saves the cache requests to file.


This makes me a bit uncomfortable; having destructors that could possibly throw (and anything that writes could definitely throw) causes headaches. But I suppose if it's only debug mode it's probably okay.

TallJimbo · 2018-03-23T19:45:57Z

include/lsst/utils/Cache.h

+    /// possible the user has some function that produces a value
+    /// when given a key, so chose to adopt that signature; any other
+    /// signature would likely require use of a lambda always.
+    Value operator()(Key const& key, std::function<Value(Key const&)>);


While it provides a nice way to document and enforce the function signature you're expecting, std::function comes with virtual function call overheads, so I usually wouldn't use it unless I need its polymorphism. Might be best to just take a template parameter here, since the implementation has to go in the header anyway. But that said, I suppose we don't cache something unless calculating it is slow enough that virtual function overheads shouldn't matter.

I went with std::function because pybind wraps it without any effort on my part (yay, calling python from C++!). I changed the C++ to template the function type for pure speed, and converted the pybind wrapper to use std::function so that's still handled simply.

TallJimbo · 2018-03-23T19:46:48Z

include/lsst/utils/Cache.h

+    /// Lookup a value
+    ///
+    /// The key must be in the cache already, or an
+    /// lsst::pex::exceptions::NotFoundError will be raised.


See DevGuide for how to document possible exceptions in Doxygen (and on that note, it would be good to document the exception safety of all of these methods).

TallJimbo · 2018-03-23T19:55:31Z

include/lsst/utils/Cache.h

+    }
+
+    std::size_t _maxElements;  // Maximum number of elements; 0 means infinite
+    mutable Container _container;  // Container of key,value pairs


I think it might be cleaner to make Container non-mutable, even if that requires more classes that use it to make their Cache data members mutable. I imagine most of those will be calling operator() rather than operator[] anyway, so the constness of operator[] doesn't gain them much.

TallJimbo · 2018-03-23T19:58:02Z

include/lsst/utils/Cache.h

+    Value operator[](Key const& key) const;
+
+    /// Add a value to the cache
+    void add(Key const& key, Value const& value);


Should document behavior if the key is already in the cache.

TallJimbo · 2018-03-23T20:04:26Z

tests/test_cache.py

+    return numberToWords(hundreds) + " hundred" + ((" " + numberToWords(rest)) if rest > 0 else "")
+
+
+class CacheTestCase(lsst.utils.tests.TestCase):


newline here.

ktlim · 2018-03-23T20:26:25Z

include/lsst/utils/Cache.h

+    void flush();
+
+#ifdef LSST_CACHE_DEBUG
+    void enableDebugging() { debuggingEnabled = true; }


Doesn't this need an underscore?

ktlim · 2018-03-23T20:29:54Z

include/lsst/utils/Cache.h

+                                _container.template project<Sequence>(it));
+        }
+#ifdef LSST_CACHE_DEBUG
+        if (debuggingEnabled) {


And here...

ktlim · 2018-03-23T20:30:22Z

include/lsst/utils/Cache.h

+    std::size_t _maxElements;  // Maximum number of elements; 0 means infinite
+    mutable Container _container;  // Container of key,value pairs
+#ifdef LSST_CACHE_DEBUG
+    bool debuggingEnabled;


TallJimbo

Looks good, thanks for the improvements!

Cache of most recently used objects, hashed by some key, with user-defined capacity limit.

Allows us to characterise the cache performance.

C++11 added std::hash, but neglected to include a function for combining multiple hashes.

TallJimbo approved these changes Mar 23, 2018

View reviewed changes

ktlim reviewed Mar 23, 2018

View reviewed changes

PaulPrice force-pushed the tickets/DM-13854 branch 5 times, most recently from ae36ebb to 4deba98 Compare March 24, 2018 04:11

TallJimbo approved these changes Mar 25, 2018

View reviewed changes

PaulPrice force-pushed the tickets/DM-13854 branch 4 times, most recently from a15fedf to 2f06bc9 Compare March 27, 2018 14:49

PaulPrice added 3 commits March 27, 2018 11:36

add Cache class

3314ade

Cache of most recently used objects, hashed by some key, with user-defined capacity limit.

Cache: add compile-time debug instrumentation

ed55179

Allows us to characterise the cache performance.

add hashCombine

e68ac28

C++11 added std::hash, but neglected to include a function for combining multiple hashes.

PaulPrice force-pushed the tickets/DM-13854 branch from 2f06bc9 to e68ac28 Compare March 27, 2018 15:36

PaulPrice merged commit e68ac28 into master Mar 27, 2018

ktlim deleted the tickets/DM-13854 branch August 25, 2018 05:57

kfindeisen mentioned this pull request Oct 29, 2018

DM-9938: Make some afw types hashable #65

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DM-13854: add general-purpose Cache class #50

DM-13854: add general-purpose Cache class #50

PaulPrice commented Mar 23, 2018

TallJimbo left a comment

TallJimbo Mar 23, 2018

TallJimbo Mar 23, 2018

TallJimbo Mar 23, 2018

TallJimbo Mar 23, 2018

TallJimbo Mar 23, 2018

PaulPrice Mar 24, 2018

TallJimbo Mar 23, 2018

TallJimbo Mar 23, 2018

TallJimbo Mar 23, 2018

TallJimbo Mar 23, 2018

ktlim Mar 23, 2018

ktlim Mar 23, 2018

ktlim Mar 23, 2018

TallJimbo left a comment

		return numberToWords(hundreds) + " hundred" + ((" " + numberToWords(rest)) if rest > 0 else "")


		class CacheTestCase(lsst.utils.tests.TestCase):

DM-13854: add general-purpose Cache class #50

DM-13854: add general-purpose Cache class #50

Conversation

PaulPrice commented Mar 23, 2018

TallJimbo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TallJimbo left a comment

Choose a reason for hiding this comment