Skip to content

Redesigned enum_<> to reduce object code bloat #1511

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

Merged
merged 3 commits into from
Sep 11, 2018
Merged

Conversation

wjakob
Copy link
Member

@wjakob wjakob commented Aug 31, 2018

This PR addresses an inefficiency in how enums are created in pybind11. Most of the enum_<> implementation is completely generic -- however, being a template class, it ended up instantiating vast amounts of essentially identical code in larger projects with many enums.

This commit introduces a generic non-templated helper class that is compatible with any kind of enumeration. enum_ then becomes a thin wrapper around this new class. The API is compatible with the previous one.

The PR consists of two portions: the first considerably extends the object_api class by adding overloads for most C++ operators that map to their Python counterparts. The second part adds the enum_-related changes, building on these operators.

With this change, the example shared library binary size goes down by 103 KiB (3.2%). It is worth noting that the examples only contain 6 enums. In larger projects with many enum data structures, savings can be substantial.

@wjakob wjakob requested review from dean0x7d and jagerman August 31, 2018 23:42
@wjakob
Copy link
Member Author

wjakob commented Aug 31, 2018

(replaces #1505)

@jagerman
Copy link
Member

jagerman commented Sep 9, 2018

Sorry, I've been busy with things lately. I'll give this a thorough go-through tomorrow.

@wjakob
Copy link
Member Author

wjakob commented Sep 9, 2018

Ok great -- thank you! By the way: what are your thoughts on pushing out a patch release with everything up to this point in 'master' (without this PR, which could go into the next minor release). Are there any other important PRs you'd want to have in either a patch release, or a minor release?

@jagerman
Copy link
Member

jagerman commented Sep 9, 2018

This looks good to me as is, but one potential idea: you could templatize enum_base with the is_arithmetic and is_convertible parameters as template parameters (and move the init() code into the enum_base constructor). That ought to allow some further reduction in binary size because the compiler could eliminate entirely the unneeded is_convertible/is_arithmetic branches (which are fairly substantial -- each of those macro expansions involves a lambda and cpp_function construction). I also expect that the is_constructible && is_arithmetic case is by far the most common (and probably the only case for a lot of projects).

On the other hand, we're still just talking about 1 (or up to 4) classes, so on any large project it is going to be a fairly negligible difference.

@jagerman
Copy link
Member

jagerman commented Sep 9, 2018

This is also going to conflict with #1453 - it might be nice to pull those changes into this PR (or merge that PR first and update this one).

This commit revamps the object_api class so that it maps most C++
operators to their Python analogs. This makes it possible to, e.g.
perform arithmetic using a py::int_ or py::array.
object_api::operator[] has a powerful overload for py::handle that can
accept slices, tuples (for NumPy), etc.

Lists, sequences, and tuples provide their own specialized operator[],
which unfortunately disables this functionality. This is accidental, and
the purpose of this commit is to re-enable the more general behavior.

This commit is tangentially related to the previous one in that it makes
py::handle/py::object et al. behave more like their Python counterparts.
This commit addresses an inefficiency in how enums are created in
pybind11. Most of the enum_<> implementation is completely generic --
however, being a template class, it ended up instantiating vast amounts
of essentially identical code in larger projects with many enums.

This commit introduces a generic non-templated helper class that is
compatible with any kind of enumeration. enum_ then becomes a thin
wrapper around this new class.

The new enum_<> API is designed to be 100% compatible with the old one.
@wjakob
Copy link
Member Author

wjakob commented Sep 11, 2018

#1453 is merged into master, and I'll shortly push out the patch release containing fixes since 2.2.3.

I looked a bit into enum specialization, and it turned out not to be worth it for my main project ("mitsuba2", which is sample_size ==1, but still .. :)). I did find a way to shave off a bit more by also moving half of the pickling code into the generic class.

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

Successfully merging this pull request may close these issues.

2 participants