2
\$\begingroup\$

I had just put together my first public project: simple(sane) new-handler for c++.

It allocates some reserved memory on start up, then releases it piece by piece. It may also raise a signal when each piece is released. It allows to monitor approaching of low memory conditions and gracefully handle them at the system level while there are still some memory available and avoid raising meaningless std::bac_alloc exceptions.

Please, review.

https://github.com/alex4747-pub/simple_new_handler

#ifndef _INCLUDE_SIMPLE_NEW_HANDLER_H_
#define _INCLUDE_SIMPLE_NEW_HANDLER_H_

#include <csignal>
#include <cstdint>
#include <cstdlib>
#include <cstring>
#include <exception>
#include <limits>
#include <new>
#include <utility>

namespace simple {

class NewHandler {
 public:
  // Initialize the driver and allocate reserved memory blocks
  //
  // If not enough memory allocate as many blocks as possible
  //
  static void Init(size_t finalBlockSize = 0, size_t reservedBlockCount = 0,
                   size_t reservedBlockSize = 0, int signo = 0) {
    GetWorker().Init(finalBlockSize, reservedBlockCount, reservedBlockSize,
                     signo);
  }

  struct State {
    size_t allocated_block_count;
    size_t available_block_count;
  };

  static State GetState() { return GetWorker().GetState(); }

  // Full state
  struct FullState {
    bool init_done;
    int signo;
    size_t final_block_size;
    bool final_block_allocated;
    size_t reserved_block_size;
    size_t reserved_block_count;
    size_t allocated_block_count;
    size_t available_block_count;
  };

  static FullState GetFullState() { return GetWorker().GetFullState(); }

 private:
  class Worker;

  static void Process() { GetWorker().Process(); }

  static Worker& GetWorker() {
    static Worker worker_;
    return worker_;
  }

  class Worker {
   public:
    Worker()
        : init_done_(),
          signo_(),
          final_block_size_(),
          reserved_block_size_(),
          reserved_block_count_(),
          allocated_block_count_(),
          available_block_count_(),
          final_block_(),
          blk_arr_list_() {}

    // Note on concurrency:
    //
    // It is a responsibility of the user to call init()
    // before entering multithreaded environment
    //
    // The environment guarantees that
    // the handler call itself is thread safe.
    //
    // Volatile is good enough for reporting purposes
    // because we do not care about race condition
    // between actual and reported usage
    //
    void Init(size_t finalBlockSize = 0, size_t reservedBlockCount = 0,
              size_t reservedBlockSize = 0, int signo = 0) {
      if (init_done_) {
        // We expect to be done once and it is done
        // more than once we do not care much
        return;
      }

      final_block_size_ = finalBlockSize;

      size_t finalSize =
          (final_block_size_ + sizeof(Blk) - 1) / sizeof(Blk) * sizeof(Blk);

      if (finalSize) {
        final_block_ = new (std::nothrow) Blk[finalSize / sizeof(Blk)];

        if (final_block_) {
          // Assign a value to map allocated block
          final_block_->m_next = 0;
        }
      }

      reserved_block_count_ = reservedBlockCount;

      unsigned int blockLimit = std::numeric_limits<unsigned int>::max();

      if (reserved_block_count_ < blockLimit)
        blockLimit = static_cast<unsigned int>(reserved_block_count_);

      reserved_block_size_ = reservedBlockSize;

      if (reserved_block_count_ && reserved_block_size_) {
        size_t reservedSize = (reserved_block_size_ + sizeof(Blk) - 1) /
                              sizeof(Blk) * sizeof(Blk);

        for (unsigned int ii = 0; ii < blockLimit; ii++) {
          Blk* blkArr = new (std::nothrow) Blk[reservedSize / sizeof(Blk)];

          if (!blkArr) {
            allocated_block_count_ = ii;
            break;
          }

          blkArr[0].m_next = blk_arr_list_;
          blk_arr_list_ = blkArr;
        }

        if (blk_arr_list_ && !allocated_block_count_) {
          // All reserved blocks were allocated
          allocated_block_count_ = blockLimit;
        }

        available_block_count_ = allocated_block_count_;
      }

      signo_ = signo;

      std::set_new_handler(NewHandler::Process);

      init_done_ = true;
    }

    State GetState() const volatile {
      State s;

      memset(&s, 0, sizeof(s));

      if (!init_done_) {
        return s;
      }

      s.allocated_block_count = allocated_block_count_;
      s.available_block_count = available_block_count_;

      return s;
    }

    FullState GetFullState() const volatile {
      FullState s;

      memset(&s, 0, sizeof(s));

      if (!init_done_) {
        return s;
      }

      s.init_done = true;
      s.signo = signo_;
      s.final_block_size = final_block_size_;
      if (final_block_) s.final_block_allocated = true;
      s.reserved_block_size = reserved_block_size_;
      s.reserved_block_count = reserved_block_count_;
      s.allocated_block_count = allocated_block_count_;
      s.available_block_count = available_block_count_;

      return s;
    }

    void Process() {
      Blk* curBlkArr = blk_arr_list_;

      if (curBlkArr) {
        // Release the first avalable block to the process
        // and raise signal if configured
        blk_arr_list_ = curBlkArr[0].m_next;

        delete[] curBlkArr;

        if (available_block_count_ > 0) available_block_count_--;

        if (signo_ != 0) std::raise(signo_);

        return;
      }

      // Release final block and terminate
      delete[] final_block_;
      final_block_ = 0;
      std::terminate();
    }

   private:
    struct Blk {
      Blk* m_next;
    };

    bool init_done_;
    int signo_;
    size_t final_block_size_;
    size_t reserved_block_size_;
    size_t reserved_block_count_;
    unsigned int allocated_block_count_;
    volatile unsigned int available_block_count_;
    Blk* final_block_;
    Blk* blk_arr_list_;
  };
};
}  // namespace simple

#endif  // _INCLUDE_SIMPLE_NEW_HANDLER_H_

\$\endgroup\$
0

1 Answer 1

2
\$\begingroup\$

My biggest bug bear:

#ifndef _INCLUDE_SIMPLE_NEW_HANDLER_H_
#define _INCLUDE_SIMPLE_NEW_HANDLER_H_

These macros are reserved for the implementation (i.e. you can't use them). An identifier with an initial underscore followed by a capitol letter is reserved for the implementation in any scope.

See: What are the rules about using an underscore in a C++ identifier?


What is the purpose of this?

  State s;

  memset(&s, 0, sizeof(s));

If you want to make sure that everything is zeroed in State object then create an appropriate constructor!


Not sure what this is for!

  if (!init_done_) {
    return s;
  }

You should design your code so it can't get to this location without it being already initialized correctly. Not sure how you can call a method on an object before it is constructed.

Since you are using a function static variable for all Worker objects creation of this object is already thread safe.


Not sure why you would do this:

  s.allocated_block_count = allocated_block_count_;
  s.available_block_count = available_block_count_;

  return s;

Why is this state not already part of the worker object as a State object. Then you can simply return the workers State object!


I would have defined the structures like this:

  struct State
  {
    size_t allocated_block_count;
    size_t available_block_count;
  };

  struct FullState {
    bool   init_done;
    int    signo;
    size_t final_block_size;
    bool   final_block_allocated;
    size_t reserved_block_size;
    size_t reserved_block_count;
    State  currentState;
  };

  class Worker
  {
        FullState fullState;
        Blk*      final_block_;
        Blk*      blk_arr_list_;

        State     GetState()     const volatile {return fullState.state;}
        FullState GetFullState() const volatile {return fullState;}
  };

This simplifies a lot of the work you have done.


The call to std::set_new_handler(NewHandler::Process); actually returns the current new_handler. You may want to retain this value and use it.

If you fail to allocate any extra memory then you call (if it is not null) the originally available handler (or put it back in place potentially).


\$\endgroup\$
19
  • \$\begingroup\$ Thanks for review. I will implement most of your notes. I do not see any point in keeping original handler - once the thing is out of memory nothing could be done. \$\endgroup\$
    – zzz777
    May 12, 2020 at 15:39
  • \$\begingroup\$ The original handler (which may not be the C++ original handler but one installed by another library) may have saved space just like your handler has done. By not calling it you have wasted that space. \$\endgroup\$ May 12, 2020 at 18:57
  • \$\begingroup\$ If there is other handlers there is simply no point of using this one. \$\endgroup\$
    – zzz777
    May 12, 2020 at 19:35
  • \$\begingroup\$ That's not true in the slightest. Libraries are know to install their own handlers for their use cases. When you have run out of memory to release you should give the other libraries a chance to do there stuff (they may be able to copy stuff to disk and release memory etc). There may be a whole chain of low memory handlers that were installed before you. If you don't call the previously installed handler you are not allowing that chain of handler to do their library specific clean up to try and reduce the problem. \$\endgroup\$ May 12, 2020 at 21:26
  • \$\begingroup\$ The only real question to ask is should you call their handler first or only call their handler if your handler has nothing else to give. \$\endgroup\$ May 12, 2020 at 21:27

Your Answer

By clicking “Post Your Answer”, you agree to our terms of service and acknowledge you have read our privacy policy.

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