4
\$\begingroup\$

I am working on a C++ project for solving a class of optimization problems. The project will be header-only, and for now, I would like to stay with C++11 (so, not fold-expressions).

To be able to support some basic logging functionality, I have started writing the below logger class. Any comment/feedback is welcome. However, I would like to ask specifically about your suggestions on how to restructure the code, if at all, to make it more efficient.

#ifndef LOGGER_HPP_
#define LOGGER_HPP_

#include <chrono>
#include <cstdint>
#include <type_traits>
#include <vector>

namespace utility {
namespace logger {

template <class float_t, bool log_x_v, bool log_g_v> struct logger_t {
  template <class InputIt1, class InputIt2>
  void operator()(const std::size_t k, const float_t fval, InputIt1 xbegin,
                  InputIt1 xend, InputIt2 gbegin, InputIt2 gend) {
    tend = std::chrono::high_resolution_clock::now();
    const auto telapsed =
        std::chrono::duration<float_t, std::chrono::milliseconds::period>(
            tend - tstart);
    iterations.push_back(k);
    times.push_back(telapsed.count());
    fvalues.push_back(fval);
    log_x(xbegin, xend, std::integral_constant<bool, log_x_v>{});
    log_g(gbegin, gend, std::integral_constant<bool, log_g_v>{});
    tstart = std::chrono::high_resolution_clock::now();
  }

private:
  template <class InputIt>
  void log_x(InputIt xbegin, InputIt xend, std::true_type) {
    xvalues.emplace_back(xbegin, xend);
  }
  template <class InputIt> void log_x(InputIt, InputIt, std::false_type) {}
  template <class InputIt>
  void log_g(InputIt gbegin, InputIt gend, std::true_type) {
    gvalues.emplace_back(gbegin, gend);
  }
  template <class InputIt> void log_g(InputIt, InputIt, std::false_type) {}

  std::vector<std::size_t> iterations;
  std::vector<float_t> times;
  std::vector<float_t> fvalues;
  std::vector<std::vector<float_t>> xvalues, gvalues;
  std::chrono::time_point<std::chrono::high_resolution_clock> tstart{
      std::chrono::high_resolution_clock::now()},
      tend;
};

template <class float_t> using value = logger_t<float_t, false, false>;
template <class float_t> using decision = logger_t<float_t, true, false>;
template <class float_t> using gradient = logger_t<float_t, false, true>;
template <class float_t> using full = logger_t<float_t, true, true>;

} // namespace logger
} // namespace utility

#endif

Basically, the class is not complete, yet. I will be adding iterator support, read/write csv/binary functionality and some locking mechanism, later on, to be able to support thread-safe logging of values inside the logger.

Specifically, what I would like to ask is if I need to have some level of abstraction to make the unused private member variables disappear. The tag dispatch on log_x and log_g member functions seem to be OK when it comes to optimizing out the empty function calls, I hope. But then, when there is no logging needed for x and g, private member variables xvalues and gvalues are redundant. Should I care about them, or should I leave it as is for the sake of maintainability and ease of reading the code?

\$\endgroup\$
  • \$\begingroup\$ Isn't "float_t" a confusing name for a template parameter? \$\endgroup\$ – Blasco Feb 28 '18 at 13:37
  • \$\begingroup\$ This is something I have heard before. OK, I need to change the name to something like real_t. The thing is, float_t will majorly be either of float or double, and real_t is a real number type, which includes integers. That was the intuition, but I need to find a better name for it, apparently. Thanks for the heads-up! :) \$\endgroup\$ – Arda Aytekin Feb 28 '18 at 13:39
  • \$\begingroup\$ you could call it value_t \$\endgroup\$ – Blasco Feb 28 '18 at 13:43
  • \$\begingroup\$ Agreed. I have started changing the name here, then ;) \$\endgroup\$ – Arda Aytekin Feb 28 '18 at 13:44
  • \$\begingroup\$ You shouldn't update the code in the question with people reviews, it's better if you leave it as it is so other people can give you their advice ;) I'm quite new to c++ so don't listen too much to me hehe. Welcome to Code Review by the way! \$\endgroup\$ – Blasco Feb 28 '18 at 13:56
0
\$\begingroup\$

You could use a better name for the template argument float_t, maybe value_t

Maybe you could combine both log functions as they are very similar:

template <typename Container, typename InputIt>
void log(Container &c, InputIt begin, InputIt end, std::true_type) {
    c.emplace_back(begin, end);
  }
\$\endgroup\$
  • \$\begingroup\$ Most likely I will have to have Container& c in the signature. That's nice, and that will definitely make my code shorter. However, still the question remains: what about the xvalues or gvalues when they are not needed? \$\endgroup\$ – Arda Aytekin Feb 28 '18 at 13:50
  • \$\begingroup\$ They would be empty vectors, would that be a big problem for you? There is probably a simpler solution. My opinion is that the code looks a little bit over complicated, why do you need the bool log_x_v, bool log_g_v template parameters, what are you gaining with it? \$\endgroup\$ – Blasco Feb 28 '18 at 13:54
  • \$\begingroup\$ Maybe my design choice is wrong, but I thought I would benefit from compile-time known parameters to implement just the enough functionality needed. I mean, if the user chooses to go for only value function logging, the call to log (in your suggestion) will get optimized out. no run-time checking whatsoever. I agree that the empty vectors should not be a big problem when one logs a couple of data points... In the end, they are simply like pointers, especially when compared to some couple of data points. \$\endgroup\$ – Arda Aytekin Feb 28 '18 at 14:01

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service, privacy policy and cookie policy

Not the answer you're looking for? Browse other questions tagged or ask your own question.