DM-13854: extend cache in Psf #334

PaulPrice · 2018-03-23T19:07:52Z

Larger cache makes things go faster.

TallJimbo

A few small comments; biggest one is to try to get Cache.h out of the header files.

Another thought: could we modify the hash function and equality comparison used by the cache for computeKernelImage to round off to the nearest integer pixel, so tiny changes in the point requested don't result in cache misses? If our PSFs are changing much over the scale of a pixel, we've got bigger problems (or we've already flagged that object with INEXACT_PSF for exactly that reason). Of course, we'd probably want to modify computeKernelImageitself to do that rounding even when not using the cache, so the cache doesn't change the actual behavior, but I think that'd be fine. And obviously we can't do that rounding in computeImage.

TallJimbo · 2018-03-23T21:14:50Z

include/lsst/afw/detection/Psf.h

-    mutable geom::Point2D _cachedImagePosition;
-    mutable geom::Point2D _cachedKernelImagePosition;
+    mutable utils::Cache<CacheKey, std::shared_ptr<Image>> _imageCache;
+    mutable utils::Cache<CacheKey, std::shared_ptr<Image>> _kernelImageCache;


Maybe shared_ptr<Image const>, because it can be?

Don't think I can, unless I cast away the same const in Psf::computeImage.

TallJimbo · 2018-03-23T21:17:00Z

include/lsst/afw/detection/Psf.h

@@ -29,6 +29,7 @@
 #include <memory>

 #include "lsst/daf/base.h"
+#include "lsst/utils/Cache.h"


As discussed on the utils ticket, I'd prefer to forward-declare Cache, have Psf hold it by unique_ptr, and move this include to the .cc file.

TallJimbo · 2018-03-23T21:20:58Z

include/lsst/afw/detection/Psf.h

+
+    /// Set the capacity of the caches
+    ///
+    /// Both the image and kernel image caches will be set to this capacity.


Use /** */ for multi-line Doxygen comments.

TallJimbo · 2018-03-23T21:21:16Z

include/lsst/afw/detection/Psf.h

+            return os << key.position;
+        }
+    };
+


Hopefully this can be moved to the .cc file, too.

Pulled it out of the class, into a preceding namespace detail. But it does mess up the top of the file a bit, especially since I have to get into namespace std to provide the std::hash support (it's more annoying to work with than boost::hash).

TallJimbo · 2018-03-23T21:22:15Z

python/lsst/afw/geom/coordinates/coordinates.cc

@@ -364,6 +366,8 @@ PyPoint<T, N> declarePoint(py::module &mod, std::string const &suffix) {
    cls.def("__ne__", [](Point<T, N> const &self, Point<T, N> const &other) { return self != other; },
            py::is_operator());

+    cls.def("__hash__", [](Point<T, N> const &self) { return boost::hash<Point<T, N>>()(self); });
+


I'm not sure it's a good idea to add this to Python; Points are not immutable and hence putting them in dicts could definitely confuse the dict.

TallJimbo

I think you didn't go quite far enough in moving things to the .cc file to reap the benefits (and doing that should greatly reduce the need for changes in other packages). More comments inline.

TallJimbo · 2018-03-25T22:12:29Z

include/lsst/afw/detection/Psf.h

+    void setCacheCapacity(std::size_t capacity) {
+        _imageCache->reserve(capacity);
+        _kernelImageCache->reserve(capacity);
+    }


If you define setCacheCapacity and getCacheCapacity (and ~Psf(), I think), you should no longer need to include Cache.h in other source files that use Psf. I'm not totally sure that's enough, but the goal here is to make sure that anything that uses Cache as more than an opaque pointer is in Psf.cc, and that will make it unnecessary to include Cache.h anywhere but Psf.cc.

TallJimbo · 2018-03-25T22:15:01Z

include/lsst/afw/detection/Psf.h

+    friend std::ostream & operator<<(std::ostream &os, PsfCacheKey const &key) {
+        return os << key.position;
+    }
+};


You'll need a forward declaration of this class in the header for the typedef later, but the rest can also go in Psf.cc.

TallJimbo · 2018-03-25T22:15:46Z

include/lsst/afw/detection/Psf.h

-    mutable image::Color _cachedKernelImageColor;
-    mutable geom::Point2D _cachedImagePosition;
-    mutable geom::Point2D _cachedKernelImagePosition;
+    typedef utils::Cache<detail::PsfCacheKey, std::shared_ptr<Image>> PsfCache;


using PsfCache = utils::Cache<detail::PsfCacheKey, std::shared_ptr<Image>> is preferred in C++11.

TallJimbo · 2018-03-25T22:16:40Z

include/lsst/afw/detection/Psf.h

-    mutable geom::Point2D _cachedKernelImagePosition;
+    typedef utils::Cache<detail::PsfCacheKey, std::shared_ptr<Image>> PsfCache;
+    mutable std::shared_ptr<PsfCache> _imageCache;
+    mutable std::shared_ptr<PsfCache> _kernelImageCache;


I believe these no longer need to be mutable; the extra layer of indirection does that for you. Since Psf is already noncopyable and immutable, std::unique_ptr should be used instead of std::shared_ptr as well.

TallJimbo · 2018-03-25T22:17:54Z

src/detection/Psf.cc

+  : daf::base::Citizen(typeid(this)),
+  _isFixed(isFixed),
+  _imageCache(new PsfCache(capacity)),
+  _kernelImageCache(new PsfCache(capacity))


Best practice is to use std::make_shared to initialize smart pointers when you're constructing the pointee at the same time (or, if you switch to std::unique_ptr, std::make_unique).

TallJimbo

Looks good!

Two tiny inline comments.

TallJimbo · 2018-03-27T13:25:02Z

include/lsst/afw/detection.h

@@ -26,6 +26,8 @@
 #ifndef LSST_AFW_DETECTION_H
 #define LSST_AFW_DETECTION_H

+#include "lsst/utils/Cache.h" // for afw::detection::Psf
+


This should be removable now.

TallJimbo · 2018-03-27T13:27:07Z

src/geom/Point.cc

@@ -21,6 +21,7 @@
 */

 #include <cmath>
+#include <boost/functional/hash.hpp>  // boost::hash


Use "", not <>.

Allows its use as a key in lsst::utils::Cache. Deliberately NOT defining Point.__hash__ in python, since Point is mutable so we don't want to be able to use it as the key in a dict.

This allows caching many more realisations (they're relatively cheap: 50x50 pixels per realisation @ 8 bytes per pixel); the number of realisations is configurable. This gains about 22% in the runtime of measureCoaddSources.py using the default 100 cache slots for all Psf instances. Allowing CoaddPsf to use 100000 cache slots increases the gain to about 25%.

TallJimbo approved these changes Mar 23, 2018

View reviewed changes

PaulPrice force-pushed the tickets/DM-13854 branch from d85aeb2 to e4cb086 Compare March 24, 2018 01:38

TallJimbo requested changes Mar 25, 2018

View reviewed changes

PaulPrice force-pushed the tickets/DM-13854 branch 3 times, most recently from 5e9f969 to 028ffca Compare March 27, 2018 01:15

TallJimbo approved these changes Mar 27, 2018

View reviewed changes

PaulPrice added 2 commits March 27, 2018 10:14

make Point hashable

2776184

Allows its use as a key in lsst::utils::Cache. Deliberately NOT defining Point.__hash__ in python, since Point is mutable so we don't want to be able to use it as the key in a dict.

PaulPrice force-pushed the tickets/DM-13854 branch from 028ffca to 9ecf299 Compare March 27, 2018 14:14

PaulPrice merged commit 9ecf299 into master Mar 27, 2018

ktlim deleted the tickets/DM-13854 branch August 25, 2018 06:44

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DM-13854: extend cache in Psf #334

DM-13854: extend cache in Psf #334

PaulPrice commented Mar 23, 2018

TallJimbo left a comment

TallJimbo Mar 23, 2018

PaulPrice Mar 24, 2018

TallJimbo Mar 23, 2018

TallJimbo Mar 23, 2018

TallJimbo Mar 23, 2018

PaulPrice Mar 24, 2018

TallJimbo Mar 23, 2018

TallJimbo left a comment

TallJimbo Mar 25, 2018

TallJimbo Mar 25, 2018

TallJimbo Mar 25, 2018

TallJimbo Mar 25, 2018

TallJimbo Mar 25, 2018

TallJimbo left a comment

TallJimbo Mar 27, 2018

TallJimbo Mar 27, 2018

DM-13854: extend cache in Psf #334

DM-13854: extend cache in Psf #334

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

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

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