Avoid calling size() in empty() member functions #1836
Conversation
Might slightly improve the speed of applications built in Debug mode. Having `empty()` call `size()` is somewhat disappointing for users who follow the guideline to replace their `size()` calls by the corresponding `empty()` calls. For example by Clang-Tidy: https://clang.llvm.org/extra/clang-tidy/checks/readability-container-size-empty.html
|
Thank you for adding this pull request to Initial Review, @StephanTLavavej Further motivation (if necessary): Users expect |
|
General note on the value of these kinds of small optimizations in some industries: http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2007/n2271.html Note that EASTL is by no means unique, it's just the only major implementation of a gaming-focused STL replacement that is available publicly of which I'm aware. (We have our own - several of our own - such libraries at Blizzard, for example.) |
Thanks for your encouraging comment, Sean! I just had a look at EASTL. Interesting! Unfortunately, it appears that EASTL also has an |
Haha, wow, that's definitely against its original spirit. |
|
Just curious: Has anyone seen benchmarks that show a perf difference or is this "only" about debug performance (which of course is an important aspect by itself)? |
|
I searched and didn't find any other occurrences of member Lines 2045 to 2048 in f675d68 Replacing this with a |
|
Thanks for improving debug codegen - and congratulations on your first microsoft/STL commit! 🎉 😸 🚀 |
Thank you for merging my pull request. That's one tiny little step for Microsoft's STL, but one giant leap for me 😸 Thanks also to the reviewers and the other participants! Cool! |
Ran Clang-Tidy (LLVM 11.1.0) `readability-container-size-empty`, as motivated at https://clang.llvm.org/extra/clang-tidy/checks/readability-container-size-empty.html : "The emptiness of a container should be checked using the empty() method instead of the size() method. It is not guaranteed that size() is a constant-time function, and it is generally more efficient and also shows clearer intent to use empty(). Furthermore some containers may implement the empty() method but not implement the size() method. Using empty() whenever possible makes it easier to switch to another container in the future." Got extra motivated by having my very first microsoft/STL commit on this topic: microsoft/STL@b95ba0e "Avoid calling size() in empty() member functions" (merged from pull request microsoft/STL#1836) Follows ITK pull request InsightSoftwareConsortium/ITK#414 commit InsightSoftwareConsortium/ITK@c82a5f2 "PERF: readability container size empty", Hans Johnson, January 2019.
Ran Clang-Tidy (LLVM 11.1.0) `readability-container-size-empty`, as motivated at https://clang.llvm.org/extra/clang-tidy/checks/readability-container-size-empty.html : "The emptiness of a container should be checked using the empty() method instead of the size() method. It is not guaranteed that size() is a constant-time function, and it is generally more efficient and also shows clearer intent to use empty(). Furthermore some containers may implement the empty() method but not implement the size() method. Using empty() whenever possible makes it easier to switch to another container in the future." Got extra motivated by having my very first microsoft/STL commit on this topic: microsoft/STL@b95ba0e "Avoid calling size() in empty() member functions" (merged from pull request microsoft/STL#1836) Follows ITK pull request InsightSoftwareConsortium/ITK#414 commit InsightSoftwareConsortium/ITK@c82a5f2 "PERF: readability container size empty", Hans Johnson, January 2019.
Might slightly improve the speed of applications built in Debug mode.
Having
empty()callsize()is somewhat disappointing for users who follow the guideline to replace theirsize()calls by the correspondingempty()calls. For example by Clang-Tidy:https://clang.llvm.org/extra/clang-tidy/checks/readability-container-size-empty.html