-
Notifications
You must be signed in to change notification settings - Fork 589
optimize sort by inlining comparison functions #17608
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
Conversation
Some benchmarks (the source code is here), "fastblead" is the one with my changes:
|
Here are some non-scientific benchmarks of numeric sort (source code):
and reverse numeric sort (source code):
As you can see, on my machine the optimized numeric sort is ~15% faster and reverse numeric sort is ~20% faster. |
I force pushed to improve a comment in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@@ -179,7 +175,7 @@ typedef SV * gptr; /* pointers in our lists */ | |||
*/ | |||
|
|||
|
|||
static IV | |||
PERL_STATIC_FORCE_INLINE IV __attribute__always_inline__ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think here and another place, the attribute__always_inline is a relic of an earlier version, and is generated by the macro. Most functions declared as such in this file don't have an attribute specified explicitly
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've added sortsv_flags_impl
to embed.fnc
and removed the attribute from its definition but I would rather not do the same for dynprep()
.
dynprep()
takes arguments of types that exist only inside pp_sort.c
, so I'd have to either move the typedefs to perl.h
(which is an obfuscation) or remove them (which is beyond the scope of this PR).
I observe ~5% overall improvement, which is very nice for such a used function. |
It's the same thing as PERL_STATIC_INLINE but it also adds __attribute__(always_inline) or __forceinline if the compiler supports that.
According to perlhack, code should be indented with four spaces. This commit doesn't contain any functional changes. If you're seeing it in "git blame" output, try using -w switch, it will hide whitespace-only changes.
That pointer isn't needed.
This will make the future changes a bit easier.
This makes special-cased forms such as sort { $b <=> $a } even faster. Also, since this commit removes PL_sort_RealCmp, it fixes the issue with nested sort calls mentioned in gh Perl#16129
@xenu I note that this is now generating |
As reported in #17632 |
This makes special-cased forms such as
sort { $b <=> $a }
even faster.
Also, since this commit removes
PL_sort_RealCmp
, it fixes theissue with nested sort calls mentioned in #16129
PS. Because of b578cab commit (normalize indentation), I recommend using "hide whitespace changes" when reviewing.