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

fix(clang-tidy): Apply all performance fixes to tests and enable performance checks on CI #3051

Merged
merged 15 commits into from
Jun 22, 2021

Conversation

Skylion007
Copy link
Collaborator

@Skylion007 Skylion007 commented Jun 20, 2021

Description

@rwgk @henryiii I went ahead and tried to fix or ignore as many of the performance issues in tests as possible. It's a big PR, but by fixing all the issues in tests or ignoring them explicitly, this should allow us to automatically run the performance checks on CI. Feel free to directly edit this PR if I broke any tests.
I also caught a couple of issues in the core library that weren't caught by the automated fixes earlier.

Types of changes
I was very conservative with what changes I made manually since I didn't want to break any testing behavior.
The changes made are mostly as follows:

  1. Add or removing missing std::move
  2. Adding noexcept
  3. Change certain variables to const ref. It's a very noisy error in tests, but it was also by far the most common performance error throughout the codebase, so I left this check enabled. I tried to fix it in several places, but others I left it as NOLINTNEXTLINE to try to avoid changing behavior of the test. Occasionally, changing it to a const ref broke things anyway due to typing errors. Many of these NOLINTNEXTLINES should probably be migrated over to proper const ref fixes if safe, but I'll let @rwgk, @henryiii or someone else more familiar with the tests down the line fix them later. It's not very high pri since testing isn't that performance sensitive. I tried to change it in examples that people might copy from test to try to demonstrate best coding practices. In other words, NOLINTNEXTLINE is the default for any test that caused issues when changing it to a const ref or I was uncertain whether it would change testing behavior. I also added it in args that are likely to be elided like static methods in tests to avoid having to change all those by hand. Feel free to get rid of as many of these as possible @rwgk .
    Some of the unparameterized nolintnextlines was because changing the class to be a multiline class or adding std::move in the constructor initializer list would break the pygrep pre-commit hook for proper upper/lowercasing.

@rwgk Plenty of tricky decisions, the most common one is not fixing the Cat and Dog classes in tests because it would cause a formatting change that would break the pygrep pre-commit hook. The other tricky decisions was mostly whether to actually fix the pass by value or leave it as a NOLINT. I erred on the side of caution with NOLINT when possible to avoid breaking tests.

Suggested changelog entry:

* Enable clang-tidy performance checks throughout the codebase to enforce performant coding practices.

@Skylion007 Skylion007 requested a review from henryiii as a code owner June 20, 2021 17:29
Copy link
Collaborator

@rwgk rwgk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's fantastic! I glanced through the changes but didn't go through with a comb, thinking that's not needed, given that this PR allows us to add automatic tooling for code health, and that we already have extensive testing. As far as I saw there are mostly or only (?) three types of changes:

  1. foo -> const foo&
  2. adding noexcept
  3. NOLINTNEXTLINE

Did I miss something important? What do you think about adding that list + anything I missed to the PR description? If you could explain (outline is fine) the rationale for deciding between adding const & vs NOLINTNEXTLINE that would be a plus. If there are any special or tricky decisions you made, pointing those out would be good, too. I will take a closer look at those.

tests/test_copy_move.cpp Show resolved Hide resolved
tests/test_eigen.cpp Show resolved Hide resolved
tests/test_exceptions.cpp Show resolved Hide resolved
tests/test_kwargs_and_defaults.cpp Show resolved Hide resolved
tests/test_local_bindings.cpp Show resolved Hide resolved
tests/test_numpy_array.cpp Show resolved Hide resolved
tests/test_numpy_vectorize.cpp Show resolved Hide resolved
@Skylion007
Copy link
Collaborator Author

@rwgk Feel free to edit the PR directly. I pointed out some of the tricky decisions I made. Mostly erred on the side of caution and just NOLINTed things instead of changing behavior.

@Skylion007
Copy link
Collaborator Author

@rwgk I also wanted to enable readability-const-return-type , but it generated a bunch of errors I am not sure how to fix such as:

../include/pybind11/detail/descr.h:77:24: error: return type 'decltype(int_to_str<1UL / 10, 1UL % 10>::digits)' (aka 'const pybind11::detail::descr<1>') is 'const'-qualified at the top level, which may reduce code readability without improving const correctness [readability-const-return-type,-warnings-as-errors]
../include/pybind11/detail/descr.h:77:24: error: return type 'decltype(int_to_str<2UL / 10, 2UL % 10>::digits)' (aka 'const pybind11::detail::descr<1>') is 'const'-qualified at the top level, which may reduce code readability without improving const correctness [readability-const-return-type,-warnings-as-errors]
../include/pybind11/detail/descr.h:77:24: error: return type 'decltype(int_to_str<32UL / 10, 32UL % 10>::digits)' (aka 'const pybind11::detail::descr<2>') is 'const'-qualified at the top level, which may reduce code readability without improving const correctness [readability-const-return-type,-warnings-as-errors]
../include/pybind11/detail/descr.h:77:24: error: return type 'decltype(int_to_str<3UL / 10, 3UL % 10>::digits)' (aka 'const pybind11::detail::descr<1>') is 'const'-qualified at the top level, which may reduce code readability without improving const correctness [readability-const-return-type,-warnings-as-errors]
../include/pybind11/detail/descr.h:77:24: error: return type 'decltype(int_to_str<4UL / 10, 4UL % 10>::digits)' (aka 'const pybind11::detail::descr<1>') is 'const'-qualified at the top level, which may reduce code readability without improving const correctness [readability-const-return-type,-warnings-as-errors]
../include/pybind11/detail/descr.h:77:24: error: return type 'decltype(int_to_str<5UL / 10, 5UL % 10>::digits)' (aka 'const pybind11::detail::descr<1>') is 'const'-qualified at the top level, which may reduce code readability without improving const correctness [readability-const-return-type,-warnings-as-errors]
../include/pybind11/detail/descr.h:77:24: error: return type 'decltype(int_to_str<64UL / 10, 64UL % 10>::digits)' (aka 'const pybind11::detail::descr<2>') is 'const'-qualified at the top level, which may reduce code readability without improving const correctness [readability-const-return-type,-warnings-as-errors]
../include/pybind11/detail/descr.h:77:24: error: return type 'decltype(int_to_str<6UL / 10, 6UL % 10>::digits)' (aka 'const pybind11::detail::descr<1>') is 'const'-qualified at the top level, which may reduce code readability without improving const correctness [readability-const-return-type,-warnings-as-errors]
/Users/agokaslan/git/my_forks/pybind11/tests/test_eigen.cpp:180:9: error: return type 'const Eigen::MatrixXd' (aka 'const Matrix<double, Dynamic, Dynamic>') is 'const'-qualified at the top level, which may reduce code readability without improving const correctness [readability-const-return-type,-warnings-as-errors]
        static const Eigen::MatrixXd createConst() { return Eigen::MatrixXd::Ones(10, 10); }
        
../include/pybind11/detail/../pytypes.h:642:5: error: return type 'pybind11::detail::generic_iterator<pybind11::detail::iterator_policies::dict_readonly>::reference' (aka 'const pair<pybind11::handle, pybind11::handle>') is 'const'-qualified at the top level, which may reduce code readability without improving const correctness [readability-const-return-type,-warnings-as-errors]
    reference operator*() const { return Policy::dereference(); }
    ^
../include/pybind11/detail/../pytypes.h:642:5: error: return type 'pybind11::detail::generic_iterator<pybind11::detail::iterator_policies::sequence_fast_readonly>::reference' (aka 'const pybind11::handle') is 'const'-qualified at the top level, which may reduce code readability without improving const correctness [readability-const-return-type,-warnings-as-errors]
../include/pybind11/detail/../pytypes.h:686:5: error: return type 'pybind11::detail::iterator_policies::sequence_fast_readonly::reference' (aka 'const pybind11::handle') is 'const'-qualified at the top level, which may reduce code readability without improving const correctness [readability-const-return-type,-warnings-as-errors]
    reference dereference() const { return *ptr; }
    ^
../include/pybind11/detail/../pytypes.h:730:5: error: return type 'pybind11::detail::iterator_policies::dict_readonly::reference' (aka 'const pair<pybind11::handle, pybind11::handle>') is 'const'-qualified at the top level, which may reduce code readability without improving const correctness [readability-const-return-type,-warnings-as-errors]
    reference dereference() const { return {key, value}; }
    ^
../include/pybind11/detail/../pytypes.h:882:5: error: return type 'pybind11::iterator::reference' (aka 'const pybind11::handle') is 'const'-qualified at the top level, which may reduce code readability without improving const correctness [readability-const-return-type,-warnings-as-errors]
    reference operator*() const {

@rwgk
Copy link
Collaborator

rwgk commented Jun 21, 2021

Hi Aaron, thanks for all the comments! I don't have the free bandwidth to get deeply into the weeds. My suggestion is to turn those github comments into terse code comments to go with the NOLINTNEXTLINE. Then over time we can weed them out, e.g. when someone is working on a related change and has built up the required context in their head already. And the cases that hang around probably aren't that important! What seems much more important to me is to get the automatic runs up and running.

@rwgk
Copy link
Collaborator

rwgk commented Jun 21, 2021

@rwgk I also wanted to enable readability-const-return-type , but it generated a bunch of errors I am not sure how to fix

No immediate idea. Sorry I don't know much about this code, too. My suggestion is to get this PR merged, then open a separate one for this issue while we have the rest in place already.

@henryiii henryiii merged commit dac74eb into pybind:master Jun 22, 2021
@github-actions github-actions bot added the needs changelog Possibly needs a changelog entry label Jun 22, 2021
rwgk pushed a commit that referenced this pull request Jun 22, 2021
rwgk pushed a commit to rwgk/pybind11 that referenced this pull request Jul 9, 2021
…lutter and clang-format issues. This was really meant to be part of PR pybind#3051 but was held back either out of an abundance of caution, or because of confusion caused by stray semicolons.
rwgk pushed a commit that referenced this pull request Jul 9, 2021
…lutter and clang-format issues. This was really meant to be part of PR #3051 but was held back either out of an abundance of caution, or because of confusion caused by stray semicolons. (#3086)
@rwgk rwgk mentioned this pull request Jul 12, 2021
@henryiii henryiii removed the needs changelog Possibly needs a changelog entry label Jul 13, 2021
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.

3 participants