fix(clang-tidy): Apply performance fixes from clang-tidy #3046

Skylion007 · 2021-06-17T21:18:27Z

Description

Spoke with @rwgk about applying performance fixes from clang-tidy which he was supportive of. While there are some blockers preventing this from being enabled automatically, (mainly the lack of a hierarchical config to prevent the test files from being edited), I went ahead and applied various fixes from the performance checks. I also formatted all diffs with clang-format-diff.py .
Most of changes can be summarized as (unnecessary std::move)
Copies that should be std::move instead
Copying in values that should be const references.
Forgetting to add noexcept to move constructors.
Let me know if any of the changes should be spun off or such because they are ABI breaking or can be applied immediately.

Suggested changelog entry:

* Various performance micro-optimizations applied to the codebase using clang-tidy.

rwgk · 2021-06-17T21:54:10Z

This is awesome!

How much trouble is it for you to apply the exact same process to the smart_holder branch?

Background:

The smart_holder branch is what we use Google-internally.
I'm working to keep it in sync with master as much as possible, but there are some non-trivial diffs.

Hope:

If you apply the exact same process to the smart_holder branch we won't have merge conflicts.

But:

I'm not sure that's actually better or easier than dealing with the merge conflicts.
In any event, I'd really want the same cleanup for the code that's only on the smart_holder branch.

Also:

I can run the Google-internal global testing (includes exercising a ton of open source code) only against the smart_holder branch.

Which means if we do master-merge followed by smart_holder-merge-from-master followed by testing, that testing may uncover some things we need to fix on master, so we have to go back and forth through the branches. If it's reasonably automatic to apply your process to both master and smart_holder straightaway, we'd have a more holistic picture.

Skylion007 · 2021-06-17T22:15:22Z

This is awesome!

How much trouble is it for you to apply the exact same process to the smart_holder branch?

Background:
* The smart_holder branch is what we use Google-internally.

* I'm working to keep it in sync with master as much as possible, but there are some non-trivial diffs.
Hope:
* If you apply the exact same process to the smart_holder branch we won't have merge conflicts.
But:
* I'm not sure that's actually better or easier than dealing with the merge conflicts.

* In any event, I'd really want the same cleanup for the code that's only on the smart_holder branch.
Also:
* I can run the Google-internal global testing (includes exercising a ton of open source code) only against the smart_holder branch.
Which means if we do master-merge followed by smart_holder-merge-from-master followed by testing, that testing may uncover some things we need to fix on master, so we have to go back and forth through the branches. If it's reasonably automatic to apply your process to both master and smart_holder straightaway, we'd have a more holistic picture.

Not that much trouble, I'll go ahead and submit a PR there as well.

rwgk · 2021-06-17T22:56:31Z

Looks like your branch is a day or two behind. Is that easy to fix? I see clang-tidy fixes in code that I changed yesterday and today. Could you please turn it into a PR against the smart_holder branch? Then I could kick off the global testing using my standard workflow.

…

On Thu, Jun 17, 2021 at 3:41 PM Aaron Gokaslan ***@***.***> wrote: @rwgk <https://github.com/rwgk> Results on SH branch: https://github.com/Skylion007/pybind11/tree/clang-tidy-sh-perf — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3046 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAFUZACG7X72LW6IA6LHDADTTJ2X7ANCNFSM464ON7IA> .

Skylion007 · 2021-06-17T23:48:29Z

Yeah, I am making a new branch with the latest fixes.

Skylion007 · 2021-06-17T23:59:29Z

@rwgk Here you go: https://github.com/Skylion007/pybind11/tree/clang-tidy-sh-perf-latest

rwgk · 2021-06-18T00:08:54Z

Awesome, that's totally up-to-date. I'm still learning the github ropes. I don't see a button to create a PR from the branch (like I do in my own fork). What's the next step for me?

…

On Thu, Jun 17, 2021 at 4:59 PM Aaron Gokaslan ***@***.***> wrote: @rwgk <https://github.com/rwgk> Here you go: https://github.com/Skylion007/pybind11/tree/clang-tidy-sh-perf-latest — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3046 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAFUZACBKXZP2SQ5PVR73GLTTKD63ANCNFSM464ON7IA> .

Skylion007 · 2021-06-18T00:15:30Z

Opened a PR assuming you want to make it in the main smart-holder branch. If you want to pull it somewhere else here is the dialogue for doing so: https://github.com/pybind/pybind11/compare/master...Skylion007:clang-tidy-sh-perf-latest?expand=1

rwgk · 2021-06-18T00:21:47Z

I see it now: #3048 That's what I was hoping for, for some reason it wasn't in the "pulls" list a few minutes ago. Thanks!!

…

On Thu, Jun 17, 2021 at 5:15 PM Aaron Gokaslan ***@***.***> wrote: Opened a PR assuming you want to make it in the main smart-holder branch. If you want to pull it somewhere else here is the dialogue for doing so: https://github.com/pybind/pybind11/compare/master...Skylion007:clang-tidy-sh-perf-latest?expand=1 — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#3046 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAFUZAD6LV2MOFEGPOASLWLTTKF27ANCNFSM464ON7IA> .

rwgk

Via #3048 I ran these changes with ASAN, MSAN, and the Google-internal global testing. I'd love to see this merged.

wjakob

That's actually pretty neat. They all look fine to me, except for one minor change I would have made.

wjakob · 2021-06-18T22:25:45Z

include/pybind11/numpy.h

@@ -164,10 +164,10 @@ struct npy_api {
            NPY_ULONG_, NPY_ULONGLONG_, NPY_UINT_),
    };

-    typedef struct {
+    using PyArray_Dims = struct {


Why not just struct PyArray_Dims?

Was here when I got here. Changed.

…g-tidy-performance-fixes

Skylion007 · 2021-06-19T16:04:12Z

include/pybind11/pybind11.h

-                    return expr;                                                       \
-                },                                                                     \
-                name(op), is_method(m_base), arg("other"))
+#define PYBIND11_ENUM_OP_STRICT(op, expr, strict_behavior)                                        \


clang-format required this change.

rwgk · 2021-06-19T16:20:40Z

include/pybind11/numpy.h

@@ -164,7 +164,7 @@ struct npy_api {
            NPY_ULONG_, NPY_ULONGLONG_, NPY_UINT_),
    };

-    using PyArray_Dims = struct {


I'm just curious, when I saw this initially I thought it was something automatically applied, thinking "probably for a good reason". Was that not the case?

It should have been automatically caught by the github actions tbh, but it wasn't. From modernize-use-using check

rwgk · 2021-06-19T17:53:51Z

Thanks Aaron!

Skylion007 added 2 commits June 17, 2021 17:08

Apply performance fixes from clang-tidy

dfc0543

2nd Round of Perf Optimizations

7ab67bc

Skylion007 mentioned this pull request Jun 17, 2021

Docs: Update py::kwargs example in function.rst to pass by reference #3038

Merged

3rd round of fixes & handle false-positive

a2149bd

henryiii changed the title ~~[Clang-Tidy] Apply performance fixes from clang-tidy~~ fix(clang-tidy): Apply performance fixes from clang-tidy Jun 18, 2021

rwgk approved these changes Jun 18, 2021

View reviewed changes

wjakob approved these changes Jun 18, 2021

View reviewed changes

Skylion007 added 3 commits June 19, 2021 11:59

Apply missing fix and clang-format

86502f3

Merge branch 'master' of https://github.com/pybind/pybind11 into clan…

9af9921

…g-tidy-performance-fixes

Apply reviewer comment

be57bda

Skylion007 commented Jun 19, 2021

View reviewed changes

rwgk reviewed Jun 19, 2021

View reviewed changes

rwgk merged commit af6218f into pybind:master Jun 19, 2021

github-actions bot added the needs changelog Possibly needs a changelog entry label Jun 19, 2021

Skylion007 deleted the clang-tidy-performance-fixes branch June 21, 2021 17:45

henryiii removed the needs changelog Possibly needs a changelog entry label Jul 13, 2021

rwgk mentioned this pull request Feb 10, 2023

FWD pybind11 google/pybind11k#3046

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(clang-tidy): Apply performance fixes from clang-tidy #3046

fix(clang-tidy): Apply performance fixes from clang-tidy #3046

Skylion007 commented Jun 17, 2021

rwgk commented Jun 17, 2021

Skylion007 commented Jun 17, 2021

rwgk commented Jun 17, 2021 via email

Skylion007 commented Jun 17, 2021

Skylion007 commented Jun 17, 2021

rwgk commented Jun 18, 2021 via email

Skylion007 commented Jun 18, 2021

rwgk commented Jun 18, 2021 via email

rwgk left a comment

wjakob left a comment

wjakob Jun 18, 2021

Skylion007 Jun 19, 2021 •

edited

Skylion007 Jun 19, 2021

rwgk Jun 19, 2021

Skylion007 Jun 19, 2021

rwgk commented Jun 19, 2021

fix(clang-tidy): Apply performance fixes from clang-tidy #3046

fix(clang-tidy): Apply performance fixes from clang-tidy #3046

Conversation

Skylion007 commented Jun 17, 2021

Description

Suggested changelog entry:

rwgk commented Jun 17, 2021

Skylion007 commented Jun 17, 2021

rwgk commented Jun 17, 2021 via email

Skylion007 commented Jun 17, 2021

Skylion007 commented Jun 17, 2021

rwgk commented Jun 18, 2021 via email

Skylion007 commented Jun 18, 2021

rwgk commented Jun 18, 2021 via email

rwgk left a comment

Choose a reason for hiding this comment

wjakob left a comment

Choose a reason for hiding this comment

wjakob Jun 18, 2021

Choose a reason for hiding this comment

Skylion007 Jun 19, 2021 • edited

Choose a reason for hiding this comment

Skylion007 Jun 19, 2021

Choose a reason for hiding this comment

rwgk Jun 19, 2021

Choose a reason for hiding this comment

Skylion007 Jun 19, 2021

Choose a reason for hiding this comment

rwgk commented Jun 19, 2021

Skylion007 Jun 19, 2021 •

edited