support for unique_ptr and significant speedups for shared_ptr vs returning const& to internal ptr_ field

[xaxxon]

If get(), operator->, and operator T(), are changed from returning a copy to returning a T const &, not_null gets support for unique_ptr (and any other move-only pointer-like object) as well as drastic speedups with pointer-like types with expensive copy constructors, like shared_ptr.

However, this means that a not-aware but potentially not malicious user could const cast the const away from the reference and change the internal value. With the recent discussion to remove the per-access null check, this would allow nulls to be returned from not_null without jumping through massive hoops.

The speed benefit to shared_ptr is drastic. In the microbenchmark code (listed below), having a not_null<shared_ptr> doubles the time to access data through it in a "getter"-type function, from 20 microseconds to 40 microseconds even when fully inline-able by the optimizer, since the reference count changes cannot be optimized out. This type of cost is associated with any non-trivial copy constructor, not just shared_ptr. With the proposed change, there is no additional cost for shared_ptr since no copy is made.

As for unique_ptr, I find the use case somewhat compelling for making sure you don't forget to allocate a pointer to it, when you don't intend for it to ever be empty. Of potentially limited use, it also stops it from being moved out of, since only a const version of the unique_ptr can ever be obtained. Without the concept of a "destructive move" in c++, there's no way to say that it's ok for the contained pointer to become null because it will never be accessed again. However, this doesn't mean there is no use case for this, just that it may be smaller than one might anticipate.

There also may be ways to achieve these goals with a more complex implementation of not_null involving much more TMP and making all the comparison operators friend functions. So I guess I'm curious what other people's thoughts are on speed versus implementation simplicity versus more heavily guaranteed safety against well-intentioned but clearly incorrect use of not_null.

My vote, as evidenced by the hacked up version of not_null I actually use is to return a const & (and I intentionally violate the contract sometimes to move out of it).

#include "GSL/include/gsl/pointers"
#include <benchmark/benchmark.h>
#include <memory>

using namespace std;

struct SomeStruct {
    int i;
};

int f(gsl::not_null<shared_ptr<SomeStruct>> nnsp) {
    return nnsp->i;
}

int f2(shared_ptr<SomeStruct> sp) {
    return sp->i;
}


static void null_test(benchmark::State& state) {
    auto nnsp = make_shared<SomeStruct>();
  for (auto _ : state) {
      for (int i = 0; i < 1000; i++)
      benchmark::DoNotOptimize(f(nnsp));
  }
}
// Register the function as a benchmark
BENCHMARK(null_test);


static void no_null_test(benchmark::State& state) {

    auto sp = make_shared<SomeStruct>();
    for (auto _ : state) {
	for (int i = 0; i < 1000; i++)
	benchmark::DoNotOptimize(f2(sp));
    }
}
BENCHMARK(no_null_test);

BENCHMARK_MAIN();

results:

2018-03-31 00:17:15
Run on (4 X 1700 MHz CPU s)
CPU Caches:
  L1 Data 32K (x2)
  L1 Instruction 32K (x2)
  L2 Unified 262K (x2)
  L3 Unified 3145K (x1)
----------------------------------------------------
Benchmark             Time           CPU Iterations
----------------------------------------------------
null_test         20129 ns      20055 ns      35072
no_null_test      19844 ns      19775 ns      34906

[xaxxon]

oops, those were the results WITH the change to return const &. Results without the change:

2018-03-31 00:13:54
Run on (4 X 1700 MHz CPU s)
CPU Caches:
  L1 Data 32K (x2)
  L1 Instruction 32K (x2)
  L2 Unified 262K (x2)
  L3 Unified 3145K (x1)
----------------------------------------------------
Benchmark             Time           CPU Iterations
----------------------------------------------------
null_test         40541 ns      40421 ns      17663
no_null_test      20216 ns      20012 ns      32823

This is compiled on my mac with clang 5

clang++ -lbenchmark main.cpp -O3 -std=c++17

Also, as a note to those who may not be familiar with google benchmark, benchmark::DoNotOptimize simply stops the call from being completely optimized away due to the optimizer knowing that the code does absolutely nothing in the grand scheme of things. It does NOT stop the resulting code from being optimized in any other way. As evidence, I present timings from a -O0 run:

2018-03-31 00:31:11
Run on (4 X 1700 MHz CPU s)
CPU Caches:
  L1 Data 32K (x2)
  L1 Instruction 32K (x2)
  L2 Unified 262K (x2)
  L3 Unified 3145K (x1)
----------------------------------------------------
Benchmark             Time           CPU Iterations
----------------------------------------------------
null_test         60049 ns      59901 ns      11762
no_null_test      38284 ns      38180 ns      18278

[xaxxon]

From the Core Guidelines:

http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#note-61

Note not_null is not just for built-in pointers. It works for unique_ptr, shared_ptr, and other pointer-like types.

[xaxxon]

ping?

[beinhaerter]

Your code calls f with a shared_ptr. Why don't you call it with a not_null<shared_ptr<..>>?

not_null works together with unique_ptr. But you have to see it another way. You don't declare a unique_ptr that is not_null, you declare a unique_ptr containing a not_null pointer. The managed pointer is not_null, not the object managing the pointer. So you write unique_ptr<not_null<..>>.

Here is some code that I tested:

#include <gsl/gsl>
#include <memory>

struct C
{
	int x;
};

int main()
{
	// Both objects can be created
	std::unique_ptr<gsl::not_null<C*>> p1{ std::make_unique<gsl::not_null<C*>>(new C()) };
	p1.get()->get()->x = 3; // this works
	gsl::not_null<std::unique_ptr<C*>> p2{ std::make_unique<C*>() };
	(*p2)->x = 3;	// Fails because not_null cannot copy return the unique_ptr
}

Edit 1: added new C() so p1 can be constructed (it worked with my MSVC 15.6.4 but fails with newer versions).
Edit 2: added main() and resolved compile issues with MSVC 15.6.4

[xaxxon]

I'm not seeing the first form work: https://godbolt.org/g/MWu4Uq

Also, the two forms aren't the same. unique_ptr<not_null<T*>> has the unique_ptr managing the not_null. When deleting a not_null, the underlying object isn't destroyed because not_null is not an owning pointer.

A not_null<unique_ptr<T*>> will destroy the underlying object when the unique_ptr stored in the not_null is destroyed.

At least I think that's how it would work, but it doesn't compile for me so I'm just trying to think through it.

Your code calls f with a shared_ptr. Why don't you call it with a not_null<shared_ptr<..>>?

Looks like I oopsed that one. I will update.

[MikeGitb]

I don't know, what you tested, but unless I'm missing something it was certainly not the code you posted.

Aside from the snippet afaik being syntactically invalid c++ code, the first version doesn't make any sense and should not compile (or at least trigger a runtime assert)

[xaxxon]

updated code to pass with a nnsp ..instead of a sp misnamed as an nnsp:

----------------------------------------------------
Benchmark             Time           CPU Iterations
----------------------------------------------------
null_test         39000 ns      38844 ns      17755
no_null_test      18618 ns      18596 ns      35415

However, the more interesting case, passing both by reference, gives not a 2x drop in speed but a 25x drop in speed using nnsp:

----------------------------------------------------
Benchmark             Time           CPU Iterations
----------------------------------------------------
null_test         19291 ns      19250 ns      36579
no_null_test        767 ns        765 ns    1107928

#include "GSL/include/gsl/pointers"
#include <benchmark/benchmark.h>
#include <memory>

using namespace std;

struct SomeStruct {
    int i;
};

int f(gsl::not_null<shared_ptr<SomeStruct>> & nnsp) {
    return nnsp->i;
}

int f2(shared_ptr<SomeStruct> & sp) {
    return sp->i;
}


static void null_test(benchmark::State& state) {
    auto sp = make_shared<SomeStruct>();
    auto nnsp = gsl::not_null<std::shared_ptr<SomeStruct>>(sp);
  for (auto _ : state) {
      for (int i = 0; i < 1000; i++)
      benchmark::DoNotOptimize(f(nnsp));
  }
}
// Register the function as a benchmark
BENCHMARK(null_test);


static void no_null_test(benchmark::State& state) {

    auto sp = make_shared<SomeStruct>();
    for (auto _ : state) {
	for (int i = 0; i < 1000; i++)
	benchmark::DoNotOptimize(f2(sp));
    }
}
BENCHMARK(no_null_test);

BENCHMARK_MAIN();

Benchmark calling a function taking a SomeStruct * also shows the same 25X+ slowdown when using a not_null<shared_ptr<>> vs a plain shared_ptr<>

These timings aren't impacted by the Ensures call in get() because in this case it can be optimized away.

[beinhaerter]

When deleting a not_null, the underlying object isn't destroyed because not_null is not an owning pointer.

This is a very good point. I'll need to double check if I use that in my code. And it makes me a bit confused about what the correct way is to use not_null and unique_ptr together.

BTW, I updated my code. It compiled with VS2017 15.6.4 but does not compile with the latest MSVC on godbolt (and not with gcc).

[MikeGitb]

@beinhaerter: I think it would be helpful, if you could post the actual code snippet (including main function) that you are using. Also, where did you get your gsl version from (github, vcpkg, nuget ...)?

In any case, as far as I understand, your solution will end up with the worst of both worlds:

• You don't have automatic memory management, because not_null<C*> is a non-owning pointer
• You don't have the non-null guarantee, because no one is preventing you from asigning null to a std::unique_ptr<gsl::not_null<C*>> (i.e.: p1 = nullptr will neither trigger a compilation, nor a runtime error).
• You have now two levels of indirection and two dynamic memory allocations instead of one.

And by the way,

p2.get()->get()->x = 3;

should actually be

p2->x = 3;

which is currently not possible, but would be with the changes proposed by xaxxon.

[beinhaerter]

@MikeGitb I updated my sample, it is now an MCVE. I had to change the access to p2 because it did not compile on MSVC 15.6.4.

[MikeGitb]

@beinhaerter: Sorry, my bad. I thought p2 was gsl::not_null<std::unique_ptr<C>> (no need for the * after C.

[ericLemanissier]

@beinhaerter your code still has double heap allocation: std::make_unique<gsl::not_null<C*>>(new C()) first allocates and constructs a C on the heap (new C()), then make_unique allocates and constructs a gsl::not_null<C*> on the heap. In the end you have a unique_ptr pointing to a not_null, itself pointing to the C instance, and you have no guarantee that the unique_ptr is not null.
I would say that having a gsl::not_null as template parameter of a smart pointer is most probably a bug.
+1 to @xaxxon 's suggestion to return by const reference instead of copy.

[beinhaerter]

@MikeGitb No, my bad. Copy-and-paste problem. not_null required C*, unique_ptr should have been C, not C*. I'll not change the sample again.
At least I have now understood that unique_ptr<not_null<>> is a bad idea. And that just because it compiles it is by no way the better solution.

@ericLemanissier

I would say that having a gsl::not_null as template parameter of a smart pointer is most probably a bug.

That's what I believe now, too. Maybe it would be good not only to have the static_assert(std::is_assignable<T&, std::nullptr_t>::value, "T cannot be assigned nullptr."); but also a check that the not_null type is not a managed pointer? I am not sure if this can be handled directly in GSL without touching the STL. The following code at least takes away everything from unique/shared_ptr that makes it a managed pointer but I bet there are better ways to do it.

namespace std {
	template<class T, class Dx>
	class unique_ptr<gsl::not_null<T>, Dx>
	{
		~unique_ptr();
	};
	template<class T>
	class shared_ptr<gsl::not_null<T>>
	{
		~shared_ptr();
	};
}

[xaxxon]

At this point it's been a couple weeks and I haven't seen anyone raise any concerns, so should I make a PR? The previous PR that was requested (in a different issue) still hasn't been merged, though, so I'm not sure what's going on.

#650

[MikeGitb]

I think you should make the PR. As this seems to be just a side project, it can sometimes take a long time until PRs get merged.

[xaxxon]

I think I updated the pull request correctly. #650

now it has the change to remove the unnecessary null check as well as allowing non-copyable pointers.

[xaxxon]

I've gone through the issue list and I think I made the PR so it will auto-resolve the related existing issues when it is accepted.

[xaxxon]

closing all my issues/PRs because GSL isn't at the maturity level of a library I can depend on.