WIP: PyPy support

[wjakob]

This commit includes a few minor modifications to pybind11 that are needed to get simple hello-world style functions to compile and run on the latest PyPy. Types are now supported as well.

The test suite compiles but crashes when executed.

[wjakob]

Related discussion on the PyPy bug tracker:

https://bitbucket.org/pypy/pypy/issues/2434/support-pybind11-in-conjunction-with-pypys
https://bitbucket.org/pypy/pypy/issues/2436/support-pybind11-in-conjunction-with-pypys

[wjakob]

A progress update here:

1. PyPy now passes 95% of the test suite 🎉 .

2. A few somewhat intrusive changes were needed to accomplish this. In the (non-PyPy) pybind11 code, when the user calls a function like def_property_static() or def_buffer(), a custom metaclass or buffer API is "snuck" into the type after it has already been created via PyType_Ready by overwriting pointers in the type data structure.

   In PyPy, we can't get away with that sort of thing anymore. The elementary type properties must be known when it is created. Thus, I've introduced two new class_<> attributes: buffer_protocol and metaclass that must be specified if the buffer protocol or static properties are used, respectively. Omitting these will lead to an (informative) exception being thrown at module import time.

   This will affect everyone, so any feedback here is welcomed.

3. NumPy integration also works. There is one memory leak in test_to_python() that is potentially a PyPy bug, I plan to file a bug. Also, test_cast_numpy_int64_to_uint64() fails for reasons unknown to me.

4. PyPy really doesn't seem to like C++ types with multiple inheritance exposed via cpyext. All of the multiple inheritance-related testcases are skipped for now.

5. PyPy doesn't seem to register class docstrings for some reason.

6. The garbage collector is a lot more finicky. I've needed to add extra gc.collect() calls in many places in the test suite to prevent the memory leak detection from detecting false positives.

7. I've inadvertently introduced some regressions, so this currently breaks some things on regular Python ;)

Cheers,
Wenzel

[dean0x7d]

Nice! That went pretty quickly from "Hello, World" to 95% of the test suite :)

Just a quick comment regarding number 6: gc.collect() returns an integer indicating the number of unreachable objects found in that pass. So the following should ensure a full cleanup:

while gc.collect():
    pass

[wjakob]

Just a quick comment regarding number 6: gc.collect() returns an integer indicating the number of unreachable objects found in that pass. So the following should ensure a full cleanup:

while gc.collect():
    pass

It doesn't work. PyPy always returns zero from gc.collect()

[wjakob]

I filed a bug report regarding the reference counting leak involving the buffer protocol: https://bitbucket.org/pypy/pypy/issues/2444/pypy-cpyext-reference-counting-leak-when

[wjakob]

PyPy bugreport regarding multiple inheritance: https://bitbucket.org/pypy/pypy/issues/2445/support-pybind11-in-conjunction-with-pypys

[mattip]

Hi. PyPY dev here. I am slowly looking into the issues raised. About "PyPy doesn't seem to register class docstrings for some reason" - once PyType_Ready is called, no changes to the c PyTypeObject struct will be reflected into app-level python. About reference counting, we do not support sys.getrefcount() at all and do not promise to collect objects that have been freed. We will not support using del as a mechanism to do anything but free resources.

And in more detail:

We create an internal app-level object when PyType_Ready is called that reflects the c-level object, we track the refcount of the c-level object and when it goes to 0 may delete both objects at our convenience but our garbage collection is a mark-and-sweep scheme, however if there is no memory pressure we may choose to not collect anything. When we do call the del method on objects, it may be during the execution of exit() from the interpreter.

We do not currently have a mechanism to update the app-level object from the c-level object (except in some special cases) after PyType_Ready is called.

[wjakob]

Hi @mattip,

cool, that's fantastic -- thank you for taking a look. Meanwhile, I've posted a followup to the multiple inheritance issue on the PyPy issue tracker.

I've created a new ticket about the issue involving tp_doc. I double-checked, and we're filling out all of the fields before the PyType_Ready call, hence it's strange that the docstrings don't show up in Python: https://bitbucket.org/pypy/pypy/issues/2446/cpyext-tp_doc-field-not-reflected-on

The design decisions regarding the cpyext and type construction make sense, and part of the changes in this PR are to make pybind11 deal with that (i.e. not rely on patching up type objects after creating them, which was always kind of hacky in any case)

Thanks,
Wenzel

[dean0x7d]

Seeing as PyPy doesn't guarantee cleanup until exit and in general GC behavior can vary based on config and platform, it may be good to disable ConstructorStats checks on PyPy. The destructor counts will likely be unreliable going from machine to machine -- no deterministic way to call gc.collect() the correct number of times. CPython's reference counting mostly plays well with C++'s deterministic destruction, but PyPy has a real GC which doesn't really make any cleanup promises.

Perhaps a global option can be added to pytest and guard all ConstructorStats checks (or replace ConstructorStats with a dummy class with a == operator which always returns true).

[wjakob]

My impression was that PyPy uses an asynchronous tri-color collector that might not have captured an object that has become garbage in the current iteration (the object might have been referenced at the beginning of that GC cycle), but it will do so in the next one at the latest. Having spent quite a while with this, my practical experience was that two calls of gc.collect() are indeed consistently enough, with the failing testcases turning out to be actual leaks. So I think there is some value in having these, at least until these assumptions are actually violated.

[wjakob]

The two main issues (multiple inheritance, docstrings) are now fixed in PyPy 🎉

That just leaves the refcounting issue involving the buffer protocol, and I think we're ready to ship this: https://bitbucket.org/pypy/pypy/issues/2444/pypy-cpyext-reference-counting-leak-when

[dean0x7d]

Slightly off-topic, but I noticed that conftest.py was reflowed to a lower line length. I'm just wondering if this is something that should be added to the flake8 configuration in order to more easily maintain the code style (flake8 is currently set to unlimited line length).

[aldanor]

Slightly off-topic, but I noticed that conftest.py was reflowed to a lower line length. I'm just wondering if this is something that should be added to the flake8 configuration in order to more easily maintain the code style (flake8 is currently set to unlimited line length).

Agreed. I vouch for linelength of 99 or 100 :)

[wjakob]

Agreed. I vouch for linelength of 99 or 100 :)

@aldanor: this sounds fine. I don't remember why I reflowed that file, probably it did not fit into my arbitrarily sized editor window :)

[wjakob]

^ @aldanor, @dean0x7d, @jagerman, @lyskov, @SylvainCorlay, @JohanMabille, @pschella

I believe that the few remaining PyPy integration issues will be addressed shortly (the PyPy devs have been super-responsive and already fixed most of the items I reported)

This means PyPy support can be part of pybind11 2.0 🎉 . Since it's a somewhat intrusive breaking change (new system and public interface for creating objects with metaclasses and supporting the buffer object protocol), it would be good to have some extra pairs of eyes looking at this PR.

Thanks,
Wenzel

[dean0x7d]

This function also appears in test_keep_alive.py and test_numpy_array.py. To avoid duplication, it's probably worth adding it to pytest_namespace in conftest.py. Then it can be called like pytest.gc_collect().

[dean0x7d]

I guess this can be merged back with the test above now that PyPy doesn't need to skip the static part.

[dean0x7d]

Same as previous comment.

[dean0x7d]

Hm, this test was being skipped but was reported as 'passed' because there were no assertion errors. Should be checked on PyPy.

[dean0x7d]

Wasn't this fixed? Leftover comment?

[lyskov]

I have tried running my project tests while using this PR code but i am running into what looks like memory error in almost all of them:

*** Error in `/usr/bin/python3': free(): invalid pointer: 0x000000000a5b6668 ***
======= Backtrace: =========
/lib64/libc.so.6(+0x7d1fd)[0x7fe440bb21fd]
/home/benchmark/rosetta/binder/main/source/build/PyRosetta/linux/clang/python-3.5/minsizerel/build/rosetta.so(+0x15ccef6)[0x7fe42842cef6]
/home/benchmark/rosetta/binder/main/source/build/PyRosetta/linux/clang/python-3.5/minsizerel/build/rosetta.so(+0x53d766d)[0x7fe42c23766d]
/lib64/libpython3.5m.so.1.0(PyEval_EvalFrameEx+0x655a)[0x7fe4419416ba]
/lib64/libpython3.5m.so.1.0(+0x12984c)[0x7fe44194484c]
/lib64/libpython3.5m.so.1.0(PyEval_EvalCodeEx+0x48)[0x7fe441944958]
/lib64/libpython3.5m.so.1.0(PyEval_EvalCode+0x3b)[0x7fe44194499b]
/lib64/libpython3.5m.so.1.0(+0x148d04)[0x7fe441963d04]
/lib64/libpython3.5m.so.1.0(PyRun_FileExFlags+0x9d)[0x7fe44196617d]
/lib64/libpython3.5m.so.1.0(PyRun_SimpleFileExFlags+0xf7)[0x7fe4419662e7]
/lib64/libpython3.5m.so.1.0(Py_Main+0xe94)[0x7fe44197c1f4]
/usr/bin/python3(main+0x169)[0x400ad9]
/lib64/libc.so.6(__libc_start_main+0xf5)[0x7fe440b56af5]
/usr/bin/python3[0x400b89]

I will see if i can obtain a better backtrace...

[lyskov]

ok on Mac OS, i can see extra message: python(89212,0x7fff79c16300) malloc: *** error for object 0x1256963f8: pointer being freed was not allocated

And i got the following backtrace:

Catchpoint 2 (exception thrown).
Catchpoint 2 (exception caught), throw location unknown, catch location unknown, exception type std::__1::bad_weak_ptr
0x00007fff8e12cac2 in __cxa_throw ()
(gdb) bt
#0  0x00007fff8e12cac2 in __cxa_throw ()
#1  0x000000010fb101be in dyld_stub_memchr ()
#2  0x000000010d837d7f in dyld_stub_memchr ()
#3  0x000000010000e50a in PyObject_Call ()
#4  0x00000001000192f7 in PyMethod_New ()
#5  0x000000010000e50a in PyObject_Call ()
#6  0x00000001000557b0 in _PyObject_SlotCompare ()
#7  0x0000000100050f87 in _PyType_Lookup ()
#8  0x000000010000e50a in PyObject_Call ()
#9  0x000000010008b399 in PyEval_EvalFrameEx ()
#10 0x0000000100088352 in PyEval_EvalCodeEx ()
#11 0x0000000100087dcb in PyEval_EvalCode ()
#12 0x000000010009cee3 in PyImport_ExecCodeModuleEx ()
#13 0x000000010009fbc2 in PyImport_AppendInittab ()
#14 0x00000001000a0663 in PyImport_AppendInittab ()
#15 0x00000001000a022d in PyImport_AppendInittab ()
#16 0x000000010009e46e in PyImport_ImportModuleLevel ()
#17 0x00000001000839b3 in _PyBuiltin_Init ()
#18 0x000000010000e50a in PyObject_Call ()
#19 0x000000010008e3df in PyEval_CallObjectWithKeywords ()
#20 0x000000010008a641 in PyEval_EvalFrameEx ()
#21 0x0000000100088352 in PyEval_EvalCodeEx ()
#22 0x0000000100087dcb in PyEval_EvalCode ()
#23 0x00000001000a800e in PyParser_ASTFromFile ()
#24 0x00000001000a7e2a in PyRun_InteractiveOneFlags ()
#25 0x00000001000a7939 in PyRun_InteractiveLoopFlags ()
#26 0x00000001000a77e3 in PyRun_AnyFileExFlags ()
#27 0x00000001000b9437 in Py_Main ()
#28 0x00007fff969ac5c9 in start ()

Please let me know if there is anything else i can do to help

[lyskov]

The Python line that lead to this is nothing special: pose = Pose() where Pose class is bound like this:

pybind11::class_<core::pose::Pose, std::shared_ptr<core::pose::Pose>> cl(M("core::pose"), "Pose", "...");
pybind11::handle cl_type = cl;
cl.def(pybind11::init<>());
cl.def(pybind11::init<const class core::pose::Pose &>(), pybind11::arg("src"));
cl.def(pybind11::init<const class core::pose::Pose &, unsigned long, unsigned long>(), pybind11::arg("src"), pybind11::arg("residue_begin"), pybind11::arg("residue_end"));
...

[wjakob]

Right, of course..

[lyskov]

looks like there is a few new commits now available in wjakob:master. After updating i indeed to longer can see any errors in both Linux and Mac.

@jagerman could it be that you missing that example is using pybind11::return_value_policy::reference_internal?

[jagerman]

@lyskov - no, the reference_internal isn't the problem, that just uses a keep-alive to delay to Container destruction until after the Data destruction. That part is working: the second Data destruction is happening when Container gets destroyed.

[jagerman]

After updating i indeed to longer can see any errors in both Linux and Mac.

That's probably just luck, the core problem is still there.

[dean0x7d]

@jagerman That issue was fixed in #533. Non-owning references don't create holders so there is no double free.

[lyskov]

hmmm, ok... then i have questions:

1. why it is working now?
2. what is the right return-policy for this case?

[lyskov]

Also, i was under the impression that object held with reference_internal will never be deleted, - is this no longer true?

[jagerman]

Ah, right, I forgot about that. So yes, it is fixed (but you'd run into it again if you returned a pointer).

[lyskov]

So, how do i need to bind function that will return pointer to object that should not be deleted?

[jagerman]

Scratch what I said, you should be fine with a pointer, too.

[lyskov]

ok, i am re-compiling my tests with an updated version of this PR and will post when i have testing results.

[dean0x7d]

For never deleting an object, there are two options:

1. A py::nodelete holder should work with any return value policy.

2. reference or reference_internal with any other holder type.

As for double delete problems, I believe those are now only a risk with the take_ownership policy.

[lyskov]

Great, thank you for clarifying this @dean0x7d ! And i will see if i can refactor my code to use py::nodelete instead of T*.

[jagerman]

Your T* holder declaration isn't doing anything anyway.

[lyskov]

@jagerman i guess it does not now, but it was indeed needed before. A while back binding like: pybind11::class_<B, B* > cl(m, "B_P"); was not compiling without it.

[dean0x7d]

Hmm, I think that right now there is actually no difference between:

py::class_<MyClass, std::unique_ptr<MyClass, py::nodelete>>(m, "MyClass")

and

py::class_<MyClass, MyClass*>(m, "MyClass")

Unless I'm overlooking something?

[jagerman]

I assume you mean with the T* holder declaration (without it you get a static assertion failure).

[dean0x7d]

Ah, I forgot about the static_assert there. So the holder declaration is not 100% useless, but probably should be avoided.

[lyskov]

allright, i just run my set of tests with a new version of source and this time i see no issues so as far as i can tell everything is working!

[wjakob]

This is merged now.

[SylvainCorlay]

Kudos on this one. This is great that it got through.

[dean0x7d]

The readme/docs say:

PyPy2.7 >= 5.5

But I think this should be >= 5.7 (future version, current nightly). Last time I checked 5.6 doesn't even compile.

[jagerman]

I wonder if it would be useful to put a static_assert testing for known-unsupported compilers. E.g. it could catch gcc < 4.8, MSVC < 2015 update 3, ICC < 15. And perhaps protected with a PYBIND11_ENABLE_UNSUPPORTED macro that people could test if they are sure they really want to bypass the checks (with the understanding that they are on their own if they do).

[wjakob]

@jagerman: sounds like a nice idea!

[wjakob]

@dean0x7d: fixed, thanks