Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Speed issue with binding from Numpy array to std::vector<Eigen::Vector3d>. #1481

Closed
syncle opened this issue Aug 2, 2018 · 5 comments
Closed

Comments

@syncle
Copy link

syncle commented Aug 2, 2018

Thanks for the great library!

I have been using pybind for our open source project. One of the useful feature we are using is binding Numpy array to std::vector<Eigen::Vector3d>. For example,

import numpy as np
import time

from pybind11_tests import Vector3dVector

class Timer(object):
    def __init__(self, name=None):
        self.name = name

    def __enter__(self):
        self.tstart = time.time()

    def __exit__(self, type, value, traceback):
        if self.name:
            print('[%s]' % self.name)
        print('Elapsed: %.3f' % (time.time() - self.tstart))

def test_stl_eigen():
    with Timer("Test Vector3dVector"):
        x = np.random.random((2000000,3))
        y = Vector3dVector(x) # for earlier than 2.2

test_stl_eigen()

in test_stl_eigen() function, y gets std::vector<Eigen::Vector3d>.

The issue I would like to mention is related to the time. We have been using pybind since 2.0_dev version. With pybind 2.0 dev, test_stl_eigen took about 0.3s. With stable release pybind 2.2 or higher, test_stl_eigen took about 2.3s. Could I ask some help from pybind community?

For your reference, here is the cpp code we wrote for binding.

#include "pybind11_tests.h"
#include "constructor_stats.h"
#include <pybind11/eigen.h>
#include <pybind11/stl.h>
#include <pybind11/stl_bind.h>
#include <pybind11/numpy.h>
#include <pybind11/operators.h>
#include <pybind11/functional.h>

PYBIND11_MAKE_OPAQUE(std::vector<Eigen::Vector3d>);

namespace pybind11 {

template <typename Vector, typename holder_type = std::unique_ptr<Vector>, typename... Args>
pybind11::class_<Vector, holder_type> bind_vector_without_repr(
        pybind11::module &m, std::string const &name, Args&&... args) {
    // hack function to disable __repr__ for the convenient function
    // bind_vector()
    using Class_ = pybind11::class_<Vector, holder_type>;
    Class_ cl(m, name.c_str(), std::forward<Args>(args)...);
    cl.def(pybind11::init<>());
    pybind11::detail::vector_if_copy_constructible<Vector, Class_>(cl);
    pybind11::detail::vector_if_equal_operator<Vector, Class_>(cl);
    pybind11::detail::vector_modifiers<Vector, Class_>(cl);
    pybind11::detail::vector_accessor<Vector, Class_>(cl);
    cl.def("__bool__", [](const Vector &v) -> bool {
        return !v.empty();
    }, "Check whether the list is nonempty");
    cl.def("__len__", &Vector::size);
    return cl;
}

template <typename EigenVector>
void pybind_eigen_vector_of_vector(py::module &m, const std::string &bind_name,
        const std::string &repr_name)
{
    typedef typename EigenVector::Scalar Scalar;
    auto vec = py::bind_vector_without_repr<std::vector<EigenVector>>(
            m, bind_name, py::buffer_protocol());
    vec.def_buffer([](std::vector<EigenVector> &v) -> py::buffer_info {
        size_t rows = EigenVector::RowsAtCompileTime;
        return py::buffer_info(
                v.data(), sizeof(Scalar),
                py::format_descriptor<Scalar>::format(),
                2, {v.size(), rows},
                {sizeof(EigenVector), sizeof(Scalar)});
    });
    vec.def("__repr__", [repr_name](const std::vector<EigenVector> &v) {
        return repr_name + std::string(" with ") +
                std::to_string(v.size()) + std::string(" elements.\n") +
                std::string("Use numpy.asarray() to access data.");
    });
    vec.def("__copy__", [](std::vector<EigenVector> &v) {
        return std::vector<EigenVector>(v);
    });
    vec.def("__deepcopy__", [](std::vector<EigenVector> &v) {
        return std::vector<EigenVector>(v);
    });
}

}

test_initializer stl_eigen([](py::module &m) {
    pybind11::pybind_eigen_vector_of_vector<Eigen::Vector3d>(m, "Vector3dVector",
            "std::vector<Eigen::Vector3d>");
});
@Fred3D-tech
Copy link

This issue is highly critical for us at the moment.

@wjakob
Copy link
Member

wjakob commented Aug 3, 2018

Hi @syncle,

could you bisect this to a specific commit? This would make our investigation much easier.

Thanks, Wenzel

@syncle
Copy link
Author

syncle commented Aug 6, 2018

Sure. This is a summary:

  • I indicated v2.1.1 (commit 1df91d3) as good (fast) and v2.2 (commit 2a5a5ec) is bad (slow).
  • I used the minimal codes provided above. The minimal code compiles well on v2.1.1 and v2.2.
  • During git bisect, I had to skip few commits, because they produced compilation errors.
  • As a result, a commit that affects the speed is one of the following commits.
    627da3f
    b68959e
    d400f60
    30d43c4

This is a full log:

jaesikpa-mac02:pybind11 jaesikpa$ git bisect start
jaesikpa-mac02:pybind11 jaesikpa$ git bisect bad 2a5a5ec
jaesikpa-mac02:pybind11 jaesikpa$ git bisect good 1df91d3

Bisecting: a merge base must be tested
[f0e58a69d382724697f9d0154e94a59529a9d512] Fix compilation with clang 4.0 in -std=c++1z mode
jaesikpa-mac02:pybind11 jaesikpa$ git checkout f0e58a69d382724697f9d0154e94a59529a9d512
HEAD is now at f0e58a6... Fix compilation with clang 4.0 in -std=c++1z mode
jaesikpa-mac02:pybind11 jaesikpa$ git bisect good
Bisecting: 117 revisions left to test after this (roughly 7 steps)
[caedf74a8943e5fe000399b2e156a4724212bd01] Fix py::make_iterator's __next__() for past-the-end calls

jaesikpa-mac02:pybind11 jaesikpa$ git checkout caedf74a8943e5fe000399b2e156a4724212bd01
HEAD is now at caedf74... Fix py::make_iterator's __next__() for past-the-end calls
jaesikpa-mac02:pybind11 jaesikpa$ git bisect bad
Bisecting: 58 revisions left to test after this (roughly 6 steps)
[b68959e822a619d1ad140f52c7197a29f4e6a06a] Use numpy rather than Eigen for copying

jaesikpa-mac02:pybind11 jaesikpa$ git checkout b68959e822a619d1ad140f52c7197a29f4e6a06a
HEAD is now at b68959e... Use numpy rather than Eigen for copying
jaesikpa-mac02:pybind11 jaesikpa$ git bisect skip
Bisecting: 58 revisions left to test after this (roughly 6 steps)
[0c4e0372a36a2389da1d661690d9358793f63829] Improve PYBIND11_DEPRECATED by showing the message on all compilers

jaesikpa-mac02:pybind11 jaesikpa$ git checkout 0c4e0372a36a2389da1d661690d9358793f63829
HEAD is now at 0c4e037... Improve PYBIND11_DEPRECATED by showing the message on all compilers
jaesikpa-mac02:pybind11 jaesikpa$ git bisect bad
Bisecting: 30 revisions left to test after this (roughly 5 steps)
[201796d94f29a6ab25628505c44c036d2dc43cad] Make any_container implicitly constructible from arithmetic values

jaesikpa-mac02:pybind11 jaesikpa$ git checkout 201796d94f29a6ab25628505c44c036d2dc43cad
HEAD is now at 201796d... Make any_container implicitly constructible from arithmetic values
jaesikpa-mac02:pybind11 jaesikpa$ git bisect good
Bisecting: 14 revisions left to test after this (roughly 4 steps)
[0a90b2db712ae8ed65f544ce28db361b11e92118] Don't let PyInstanceMethod hide itself

jaesikpa-mac02:pybind11 jaesikpa$ git checkout 0a90b2db712ae8ed65f544ce28db361b11e92118
HEAD is now at 0a90b2d... Don't let PyInstanceMethod hide itself
jaesikpa-mac02:pybind11 jaesikpa$ git bisect good
Bisecting: 7 revisions left to test after this (roughly 3 steps)
[083a0219b5962a146a5e1556d7ce5e5187cb8bca] array: implement array resize

jaesikpa-mac02:pybind11 jaesikpa$ git checkout 083a0219b5962a146a5e1556d7ce5e5187cb8bca
HEAD is now at 083a021... array: implement array resize
jaesikpa-mac02:pybind11 jaesikpa$ git bisect good
Bisecting: 3 revisions left to test after this (roughly 2 steps)
[627da3f135d3e01396f4ae911dc15145c1856cf0] Making a copy when casting a numpy array with negative strides to Eigen.

jaesikpa-mac02:pybind11 jaesikpa$ git checkout 627da3f135d3e01396f4ae911dc15145c1856cf0
HEAD is now at 627da3f... Making a copy when casting a numpy array with negative strides to Eigen.
jaesikpa-mac02:pybind11 jaesikpa$ git bisect skip
Bisecting: 3 revisions left to test after this (roughly 2 steps)
[d400f60c96312f32fafcb15168bbf56970ff3d3a] Python buffer objects can have negative strides.

jaesikpa-mac02:pybind11 jaesikpa$ git checkout d400f60c96312f32fafcb15168bbf56970ff3d3a
HEAD is now at d400f60... Python buffer objects can have negative strides.
jaesikpa-mac02:pybind11 jaesikpa$ git bisect skip
Bisecting: 3 revisions left to test after this (roughly 2 steps)
[30d43c4992f90ac7592fdbedd56c332717278c91] Now `shape`, `size`, `ndims` and `itemsize` are also signed integers.

jaesikpa-mac02:pybind11 jaesikpa$ git checkout 30d43c4992f90ac7592fdbedd56c332717278c91
HEAD is now at 30d43c4... Now `shape`, `size`, `ndims` and `itemsize` are also signed integers.
jaesikpa-mac02:pybind11 jaesikpa$ git bisect bad
Bisecting: 2 revisions left to test after this (roughly 2 steps)
[2b941b38b403bc5b7a6d065e57c972937ccf40ee] Add missing header to setup.py

jaesikpa-mac02:pybind11 jaesikpa$ git checkout 2b941b38b403bc5b7a6d065e57c972937ccf40ee
HEAD is now at 2b941b3... Add missing header to setup.py
jaesikpa-mac02:pybind11 jaesikpa$ git bisect good
There are only 'skip'ped commits left to test.
The first bad commit could be any of:
627da3f135d3e01396f4ae911dc15145c1856cf0
b68959e822a619d1ad140f52c7197a29f4e6a06a
d400f60c96312f32fafcb15168bbf56970ff3d3a
30d43c4992f90ac7592fdbedd56c332717278c91
We cannot bisect more!

@syncle
Copy link
Author

syncle commented Aug 31, 2018

Hello. Could somebody follow-up on this? I would be appreciated. :)

@syncle
Copy link
Author

syncle commented Nov 9, 2018

We got a great feedback from @wjakob, and we were able to improve performance a lot.

Here is a detailed description written by @yxlao in our Open3D repository. The key idea was

to avoid casting millions of small vectors, which requires a proportional amount of Python API calls.

For example we defined new functions like below.

template <typename EigenVector>
std::vector<EigenVector> py_array_to_vectors_double(
        py::array_t<double, py::array::c_style | py::array::forcecast> array) {
    size_t eigen_vector_size = EigenVector::SizeAtCompileTime;
    if (array.ndim() != 2 || array.shape(1) != eigen_vector_size) {
        throw py::cast_error();
    }
    std::vector<EigenVector> eigen_vectors(array.shape(0));
    auto array_unchecked = array.mutable_unchecked<2>();
    for (size_t i = 0; i < array_unchecked.shape(0); ++i) {
        // The EigenVector here must be a double-typed eigen vector, since only
        // open3d::Vector3dVector binds to py_array_to_vectors_double.
        // Therefore, we can use the memory map directly.
        eigen_vectors[i] = Eigen::Map<EigenVector>(&array_unchecked(i, 0));
    }
    return eigen_vectors;
}
 template <typename EigenVector>
std::vector<EigenVector> py_array_to_vectors_int(
        py::array_t<int, py::array::c_style | py::array::forcecast> array) {
    size_t eigen_vector_size = EigenVector::SizeAtCompileTime;
    if (array.ndim() != 2 || array.shape(1) != eigen_vector_size) {
        throw py::cast_error();
    }
    std::vector<EigenVector> eigen_vectors(array.shape(0));
    auto array_unchecked = array.mutable_unchecked<2>();
    for (size_t i = 0; i < array_unchecked.shape(0); ++i) {
        eigen_vectors[i] = Eigen::Map<EigenVector>(&array_unchecked(i, 0));
    }
    return eigen_vectors;
}

Thanks for the feedback and help!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants