-
-
Notifications
You must be signed in to change notification settings - Fork 18.3k
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
Added clang inline helper #30990
Added clang inline helper #30990
Conversation
makes sense, looking forward to the perf stats |
Doesn't clang define |
From what I understand clang defines it but the semantics of the |
Have you checked whether binaries produced before this patch and with this patch are different? I dont' see why |
I tried running the entire benchmark suite but got stuck at 30% somewhere over night, so re-ran just for indexing this morning. Here are the results: before after ratio
[8ff2ebd9] [77a96005]
<axis-cleanup> <clang-inline>
- 5.05±0.2ms 4.31±0.03ms 0.85 indexing_engines.ObjectEngineIndexing.time_get_loc('non_monotonic')
- 179±2μs 151±7μs 0.85 indexing.NumericSeriesIndexing.time_getitem_scalar(<class 'pandas.core.indexes.numeric.Int64Index'>, 'nonunique_monotonic_inc')
- 24.4±1ms 17.1±0.5ms 0.70 indexing.NumericSeriesIndexing.time_loc_scalar(<class 'pandas.core.indexes.numeric.UInt64Index'>, 'nonunique_monotonic_inc')
- 44.4±3ms 26.0±2ms 0.59 indexing.InsertColumns.time_assign_with_setitem
SOME BENCHMARKS HAVE CHANGED SIGNIFICANTLY.
PERFORMANCE INCREASED. So I think a measurable boost in some places |
any firm conclusions about the perf impact? |
Yea looks to give a boost to indexing operations. I didn't run the entire test suite because this inline helper is already generated by Cython so already used in anything but our custom C code |
Planning to merge at the end of the day tomorrow unless any other comments @pandas-dev/pandas-core |
That is part of this, but the MSVC inline could become Do any of these compilers have different behavior for vendor-specific keywords rather than C99 |
Should be first? GNUC says: "If you are writing a header file to be included in ISO C90 programs, write inline instead of inline. See Alternate Keywords." |
Not sure, but I don't know if worth straying from what Cython generates for this in any case |
i guess this can't hurt. |
gcc on macOS I think executes clang more often than not, and it appears we didn't have a inline helper defined for that. This is ported from
CYTHON_INLINE
which essentially does the same in cythonized files, save the static declarationThis might add a bit of build time to macOS (I think partially explains why it's a few minutes faster than the rest) but makes it more similar in performance to the others
Running the full benchmark suite to see where this may help