Improve constructor/destructor tracking

[jagerman]

This commit rewrites the examples that look for constructor/destructor
calls to do so via static variable tracking rather than output parsing.

The added constructor_stats class provides methods to keep track of
constructors and destructors, number of default/copy/move constructors,
and number of copy/move assignments. It also provides a mechanism for
adding a value (e.g. for value construction), and then allows all of
this to be checked at the end of a test.

By not relying on the precise pattern of constructions/destructions,
but rather simply ensuring that every construction is matched with a
destruction on the same object, we ensure that everything that gets
created also gets destroyed as expected.

With this change, relaxed mode is no longer needed, which enables
testing for proper destruction under MSVC, and under any other compiler
that generates code calling extra constructors, or optimizes away any
constructors.

[jagerman]

I should also mention that this PR is very much a companion PR to #321.

[dean0x7d]

Was this entire file accidentally included from PR #322?

[dean0x7d]

I very much agree that it's important to check proper destruction on MSVC and other compilers.
There are generally two problem areas with constructor tracking:

1. Copy elision is C++ implementation-defined

This is the reason for the original relaxed mode. Your implementation is an improvement since it allows copy/move assignments and constructor/destructor pairs to be tracked on all compilers. However, copy/move constructors are not tracked at all so this is a compromise between the default, completely strict mode and the relaxed "don't check anything" mode.

I'm not sure how critical it is to strictly track copy/move constructors. It's important for the performance of expensive-to-copy objects, so I can see it being useful to have at least some tracking, even if it's just for a specific compiler implementation. On the other hand, with copy elision things can change even between compiler versions, so things can get tricky. C++17 is set to adopt guaranteed copy elision which may even out the compilers, but produce difference compared to C++14 mode on the same compiler.

2. Destructor calls are Python implementation-defined

We're kind of lucky that the CPython implementation is reasonably predictable and it calls destructors as expected in the tests. But the entire refcounting business is an implementation detail and Python itself doesn't really guaranteed it in any way. It may be nice to have an escape hatch to turn off destructor checks quickly if the need arises.

[wjakob]

This PR makes the test script output less "noisy" but on the other hand makes the example plugin somewhat more complicated (local attribute plus multiple function calls for every declared class), which I find non-ideal.

I wonder if there is a middle ground: perhaps there could be a base class which is inherited using the curiously recurring template pattern?

template <typename Derived> class ReferenceTracker {
    ReferenceTracker() {
         ... keep track of newly created instance `this` of type typeid(Derived) ...
    }
    ReferenceTracker(const ReferenceTracker &) {
         ... keep track of copied instance `this` of type typeid(Derived) ...
    }
    ReferenceTracker(const ReferenceTracker &&) {
         ... keep track of move constructed instance `this` of type typeid(Derived) ...
    }
    ~ReferenceTracker() {
         ... deallocate instance `this` of type typeid(Derived) ...
    }
};

Any class X in the example codebase would then inherit from ReferenceTracker<X>

[jagerman]

However, copy/move constructors are not tracked at all

They are actually tracked (and pretty much have to be to avoid having too many destructions): the constructor_stats.move_created()/.copy_created() calls do that. The tests no longer check the specific number of move/copy constructors, because that's exactly the thing that varies across compilers. However, each constructor--including the copy/move constructors--has to be matched with a destructor for the test to succeed. Copy ellision is going to reduce the number of copy/move constructors, but is also going to reduce the number of destructors, so the total remains balance.

It's important for the performance of expensive-to-copy objects, so I can see it being useful to have at least some tracking

For tests where it is important, checking cstats.copy_constructions or cstats.move_constructions is possible--e.g. perhaps a test against cstats.copy_constructions == 0. (I forgot to expose this to python, 0c5337f fixes that).

We're kind of lucky that the CPython implementation is reasonably predictable and it calls destructors as expected in the tests.

We really aren't lucky in that respect—the current master test code doesn't test the order/timing of destructions at all, merely that they happen eventually (the test output gets sorted before being compared). This PR improves on that aspect of the tests quite a bit by invoking gc.collect() inside cstats.alive() to try to reduce the amount of luck required and make the whole thing more predictable. (And actually, now that I think about it, the sorting of output may no longer be needed as well; I shall investigate).

[wjakob]

(I originally added sorting to deal with the random deallocation order caused by the garbage collector)

[wjakob]

It would be nice to track that move constructors are being used in places that define them. The tests could be non-strict, i.e. ">= 1" rather than " == 2" in cases where the number depends on the compiler.

[dean0x7d]

[copy/move constructors] They are actually tracked (and pretty much have to be to avoid having too many destructions)

Yeah, sorry, I didn't word my last message quite correctly. I just meant that the count isn't checked like the existing tests, which would be nice for performance reasons. Perhaps it would be enough to ensure num_moves > num_copies (where needed) without hard numbers, as @wjakob suggested.

[destructors] This PR improves on that aspect of the tests quite a bit by invoking gc.collect() inside cstats.alive() to try to reduce the amount of luck required and make the whole thing more predictable.

There's actually no language level guarantee that the refcount will go to zero at that point (this also affects the way destructors are triggered in PR #321). The interpreter could hold some internal reference which could prevent destruction before the end of the program. We're lucky that CPython doesn't actually do that, so it works in practice, but there's actually no guarantee even after calling the GC. Not that we have any better alternative, I just wanted to make a note of it.

(And actually, now that I think about it, the sorting of output may no longer be needed as well; I shall investigate).

The set and dict tests still require output sorting, e.g. here.

[jagerman]

I wonder if there is a middle ground: perhaps there could be a base class which is inherited using the curiously recurring template pattern?

I don't think that pattern will help much, because every class still needs to define all the constructors, which themselves have to be sure to invoke : ReferenceTracker(std::move(m)) and similar. (Or, if they don't do anything, you'll at the very least need each declared with a = default;)

However, I have another solution that should simplify things--I'll push it shortly.

[jagerman]

I'm not quite sure how to write the tests for move vs copy constructor counts; I think that testing the actual number of copy constructors will work, but specifying a test on the number of move constructors is going to be unreliable.

It would be nice to track that move constructors are being used in places that define them

I thought testing for any vs. none might work, but apparently not: MSVC invokes a move constructor (of the ref class) that gcc never hits (presumably because it elides the call).

[wjakob]

GCC/Clang can be used as a reference for the minimum number of move constructor calls. MSVC may use extra move/copy constructor calls.

[jagerman]

GCC/Clang can be used as a reference for the minimum number of move constructor calls. MSVC may use extra move/copy constructor calls.

Okay, I'll do that. That may require some maintenance going forward now and then—I found a case while working on the implicit-cpp-conversion branch where g++ 6 elided a move constructor that g++ 5 didn't. But that's probably relatively rare.

[jagerman]

The set and dict tests still require output sorting

And actually more than: some of the .ref file output order differs from the Python's output order (perhaps because it was run on a python with different output buffering). Anyway, PR #321 seems much better equipped to handle doing away with the sorting (except for the specific set/dict/kwargs tests, of course).

[dean0x7d]

In Python 3 hashes are randomized between runs which makes sorting a must for set/dict.

[dean0x7d]

I've been working on PR #321 (pytest) and I've ported all of the tests except for the constructor-heavy ones (e.g. smart_ptr). The constructor tracking introduced in PR #324 is a much nicer approach for those tests. To avoid duplicating work for either PR, it would be best to merge PR #324 first since it seems to be pretty much done (the set/dict ordering issues are handled in PR #321). There's no hurry at all, just FYI.

[wjakob]

Hi Jason,

I took a look at your modified patch and still find it too heavyweight -- it adds some "magic" instrumentation (at least it may seem this way to new users) to each class that may be confusing to people who look at the tests as an additional kind of documentation on using the library in practice. Macros like DEF_CSTATS_MEMBER that replace standard library constructs are similarly problematic.

I realize that the previous "cout" calls were annoying as well but it was at least clear that they were basically just annotations which did not affect the functionality of the underlying code. I've always found looking at the sequence of constructor calls while running the example programs extremely instructive. It makes it really clear how pybind11 (temporally!) talks to the the bound C++ code, and this all goes away with this change.

IMHO this doesn't bring enough to the table to justify axing the current approach, which basically works nicely on all platforms (with the relaxed mode hack on Windows, which is just one of many other hacks that is needed to deal with that platform).

I'm not sure how to proceed -- maybe it's too hard to reconcile all of these different aspects. Thoughts?

Best,
Wenzel

[jagerman]

I think we could address them in a couple of ways:

1. Adding print_created(...)/print_destroyed()/etc. that prints (in addition to tracking) so that the constructor output information is preserved in examples where it's useful to preserve it. In some cases it isn't particularly useful as example output, but tracking that construction/destruction is happening as desired is a useful test. For testing purposes we'd still get the tracking information, but for output purposes we could just ignore all the constructor/destructor output.
2. PR Port test suite to pytest #321 ought to be able to get rid of all of the actual output of number of constructors in favour of just testing their values so that the bits of the .py printing the number of constructors (which really isn't useful as an example, but is just about testing) can go.
3. I think it ought to be possible to remove the DEF_CSTATS_... macros in favour of a single function exported from example.cpp that takes the class as an argument, so that the definitions can be removed from the example classes.

With those changes, the only things left in the examples would be constructors/destructors containing either a print_...() or track_...() depending on whether printing the constructor/destructors are useful, and then the .py code to test that the number of constructions/destructions/etc. is acceptable.

[wjakob]

Just thinking out loud, not sure if this is useful:

1. One super-easy way to assert that there are no leaks is to check py::detail::get_details().registered_instances.size() remains unchanged after running the test suite. (presumably after running the GC once). This does nothing to ensure that move constructors are executed though.
2. Throughout the test suite, there are actually relatively few move constructor calls (in contrast to normal constructors and copy constructors). It's so few of them, in fact, that it may be straightforward to just keep the "cout << " calls for these and test for their presence explicitly (even in the proposed PR Port test suite to pytest #321). In cases where multiple solutions are admissible, the test suite could check for them all.

[jagerman]

One super-easy way to assert that there are no leaks is to check py::get_details().registered_instances.size() remains unchanged

That's only one aspect of what we check: it wouldn't let us easily tell that the right number of objects were constructed and that we didn't somehow get (for example) unintended extra default constructions. It also wouldn't help checking ref class life cycles.

It's so few of them, in fact, that it may be straightforward to just keep the "cout << " calls for these and test for their presence explicitly

Because move constructors can be elided, it isn't enough to check that move constructors were called, we have to also test that no extra copy constructors were called, which means we need to track the number of copy constructors in order to test that the move constructors are working as expected.

[jagerman]

I think my latest commit addresses your concerns, Wenzel. It re-adds the constructor/destructor printing for example purposes, gets rid of the necessity to declare a member to access that (instead access is via a static ConstructorStats.get method that takes the desired class), and the constructors now look like:

MyClass() { print_default_created(this); }

which prints and tracks; the printing for example output, the tracking for constructor testing.

So, basically, from the point of an individual example, the change in most cases is just changing std::cout << "Constructed MyClass @ " << this << std::endl; to print_default_created(this), while maintaining the tracking that lets us track that everything is working as expected.

There are a couple cases that still need a bound method to access constructor stats: basically wherever the stats is on an unregistered C++ instance, such as Payload in example-callbacks, and the ref class in example-smart-ptr.

I also added a "### " prefix to all of the constructor/destructor/assignment output because I find it makes it considerably easier to follow the example output (it also makes it trivial to ignore all '### ' lines when testing).

[jagerman]

MSVC seems to be failing because of output buffer collisions between C++ and python; can we change the build tests to run python with "-u" to not buffer output?

[wjakob]

Yes, that would be perfectly reasonable.

[jagerman]

That didn't fix it, I'm guessing because the subprocess.check_output call is where the buffering is happening.

[wjakob]

I don't think you can generally rely on things being immediately garbage collected when you assign None to a local. Invoking the garbage collector might be prudent.

[jagerman]

Indeed, but rather than have gc.collect() calls all over the place, the first thing cstats.alive() does is a garbage collector invocation.

[wjakob]

Gotcha!

[wjakob]

Thanks for the updated version -- I added a few more comments. Apart from those, is this patch ready to be merged? (i.e. all examples converted?)

[jagerman]

Thanks for the updated version -- I added a few more comments. Apart from those, is this patch ready to be merged? (i.e. all examples converted?)

Yes, it's ready to go (as soon as re-add the ref pointer values, squash and rebase).

[dean0x7d]

If I understood correctly, the constructor messages are now printed, but not checked at all (with any compiler). So they are only informative and intended for people who manually run the individual examples? Should this also apply for #321, i.e. don't check the constructor output, but let it print?

[wjakob]

I think the trick will be to ignore the lines prefixed with "###" and test the remaining lines which summarize overall statistics about the instances created&destroyed.

[jagerman]

If I understood correctly, the constructor messages are now printed, but not checked at all (with any compiler).

Right. (The constructor are still checked via constructor counting, of course; but yes, the printed messages are strictly for human consumption).

[jagerman]

... and test the remaining lines which summarize overall statistics about the instances created&destroyed.

Or even easier: delete those lines and just assertion-check the values in the associated PR #321 test.

[dean0x7d]

Essentially:

# OLD
with capture:
    a = A()
assert capture.relaxed == "Constructor message"
...  # code
with capture:
    del a
assert capture.relaxed == "Destructor message"

will become:

# NEW
a = A()
...  # code
del a

cstats = ConstructorStats.get(A)
assert cstats.alive() == 0

[wjakob]

(And in a few places, a check that a move constructor was used?)

[jagerman]

You can probably use the values in the tests now for checking number of different types of constructors, and for testing any constructors that stash a value in cstats.values(), but yes, what you wrote is the basic idea.

[dean0x7d]

Yes, I'd just port all the checks exactly as implemented in this PR, e.g.:

assert cstats.move_constructions >= 1
assert cstats.copy_constructions == 3
# etc.

[jagerman]

Yup, sounds good!

[jagerman]

ref<> constructors/assignments now print the assigned pointers, I documented how to use the class at the top of constructor-stats.h, and squashed and rebased it against current master. This should be ready to go now.

[wjakob]

Awesome -- thank you very much for the good work.