CODEWITHSUNDEEP
c++segmentation-faultunique-ptr
Ebstein
Ebstein

Reputation: 31

placement new unique pointer

It's one of my first posts. I hope it contains the correct, minimal information. Please let me know if I omitted anything.

I'm trying to use placement new to increase the efficiency of the following chunk of code (I want to reduce the number of memory allocation calls since the number_of_actions variable is large (> 500k)).

First, I am using two classes whose relationship can be summarized as: 

class txn {
public:
  int i;
  txn(): i(0) {};
};

class ActionClass {
private:
  txn* t;
public:
  ActionClass(txn* t): t(t) {};
  ~ActionClass() { delete t; }
};

The code I was using originally to create an array of pointers to objects:

std::vector<std::unique_ptr<IBatchAction>> allocate_actions(unsigned int number_of_actions) {
  std::vector<std::unique_ptr<IBatchAction>> res;
  for (unsigned int i = 0; i < number_of_actions; i++) {
    // construct the action
    std::unique_ptr<IBatchAction> act = std::make_unique<ActionClass>(new TestTxn());

    res.push_back(std::move(act));
  }
  return res;
}

Code after changing to use placement new:

std::vector<std::unique_ptr<IBatchAction>> allocate_actions(unsigned int number_of_actions) {
  std::vector<std::unique_ptr<IBatchAction>> res(number_of_actions);

  // allocate all of the memory for actions up front to amortize the cost.
  ActionClass* actions = 
    reinterpret_cast<ActionClass*>(new char[number_of_actions * sizeof(ActionClass)]);
  txn* txns = reinterpret_cast<txn*>(new char[number_of_actions * sizeof(TestTxn)]);

  // use placement new to initialize actions and assign them to unique_ptrs
  for (unsigned int i = 0; i < number_of_actions; i++) {
    // construct the action using placement new from the memory allocated above.
    res[i].reset(new(&(actions[i])) ActionClass(new(&(txns[i])) TestTxn()));
  }
  return res;
}

Within main.cpp I just call the above functions multiple times, time it and return 0. The vector returned from the above function is destroyed in between loop iterations. As a result I get the following stack trace just before segfault:

#0  std::unique_ptr<IBatchAction, std::default_delete<IBatchAction> >::~unique_ptr (
    this=<optimized out>, __in_chrg=<optimized out>) at /usr/include/c++/5/bits/unique_ptr.h:236
#1  std::_Destroy<std::unique_ptr<IBatchAction, std::default_delete<IBatchAction> > > (
    __pointer=<optimized out>) at /usr/include/c++/5/bits/stl_construct.h:93
#2  std::_Destroy_aux<false>::__destroy<std::unique_ptr<IBatchAction, std::default_delete<IBatchAction> >*> (__last=<optimized out>, __first=0x7ffff7f06018)
    at /usr/include/c++/5/bits/stl_construct.h:103
#3  std::_Destroy<std::unique_ptr<IBatchAction, std::default_delete<IBatchAction> >*> (
    __last=<optimized out>, __first=<optimized out>) at /usr/include/c++/5/bits/stl_construct.h:126
#4  std::_Destroy<std::unique_ptr<IBatchAction, std::default_delete<IBatchAction> >*, std::unique_ptr<IBatchAction, std::default_delete<IBatchAction> > > (__last=0x7ffff7fc9510, 
    __first=<optimized out>) at /usr/include/c++/5/bits/stl_construct.h:151
#5  std::vector<std::unique_ptr<IBatchAction, std::default_delete<IBatchAction> >, std::allocator<std::unique_ptr<IBatchAction, std::default_delete<IBatchAction> > > >::~vector (this=0x7fffffffd910, 
    __in_chrg=<optimized out>) at /usr/include/c++/5/bits/stl_vector.h:424
#6  time_workload_creation (exp_conf=...) at start_batch/main.cc:18
#7  0x000000000040320c in main (argc=<optimized out>, argv=<optimized out>)
    at start_batch/main.cc:44

I'm new to the concept of placement new. The following is what I have used to write the above code:

  1. Can I use placement new to reset an object within a shared_ptr? -- usage of reset to assign a new object created using placement new
  2. placement new on shared_ptr make seg fault when delete -- a very similar problem. The answers to this one did one help me at all :(

I'm likely doing something obviously wrong, but I can't figure it out. Help? If you need the actual code from main (simplified), here it is (actions.h contains the function I have discussed). 

#include "actions.h"

#include <chrono>
#include <vector>

void time_workload_creation(unsigned int act_num) {
  std::chrono::system_clock::time_point time_start, time_end;
  std::vector<double> results;
  for (unsigned int i = 0; i < 10; i++) {
    time_start = std::chrono::system_clock::now();
    auto workload = allocate_actions(act_num);
    time_end = std::chrono::system_clock::now();
    results.push_back(
        std::chrono::duration_cast<std::chrono::milliseconds>(time_end - time_start).count());
  }

  for (unsigned int i = 0; i < 10; i++) {
    std::cout << i << "\t\t" <<  
      act_num << "\t\t" <<  
      results[i] << "\t\t" <<  
      act_num / results[i] << std::endl;
  }
};

int main(int argc, char** argv) {
  time_workload_creation(1000000);
  return 0;
}

Compiled using: gcc version 5.4.0 20160609 (Ubuntu 5.4.0-6ubuntu1~16.04.4)

Upvotes: 2

Views: 3665

Answers (1)

Jonathan Wakely
Jonathan Wakely

Reputation: 171403

Code after changing to use placement new:
[...]

This code has undefined behaviour. You create an array of char using new[] and then reset N unique_ptr objects which will try to delete N different ActionClass objects. How do you expect that to work? You need to use delete[] to free the array of char, not use N delete operations on chunks of the char array.

Similarly, each ActionClass will try to delete its txn object, but that's also totally undefined, because you didn't allocate N txn objects, you allocated another array of char.

That's not how memory allocation works. You can't allocate a big chunk and then deallocate pieces of it. (Well, you can, but only by writing your own custom (de)allocation functions, or allocator).

Upvotes: 3

Related Questions