Force inline boost::gregorian::date::date(special_values) to improve performance #122
Conversation
At least GCC 6.5.0, 7.4.0 and 8.3.0 fail to inline gregorian::date::date(special_values) when constructing posix_time::ptime and possibly other types. This leads to large performance overheads for a operation that should ideally be a store of a constant to some memory location.
p12tic
force-pushed
the
forceinline-gregorian-date-special-values-constructor
branch
from
b861d0e
to
0c7c332
Compare
Codecov Report
@@ Coverage Diff @@
## develop #122 +/- ##
===========================================
- Coverage 53.41% 53.33% -0.09%
===========================================
Files 83 83
Lines 4944 4933 -11
Branches 2407 2397 -10
===========================================
- Hits 2641 2631 -10
Misses 420 420
+ Partials 1883 1882 -1
Continue to review full report at Codecov.
|
|
This function is already To verify in this particular case, I've tested |
|
@JeffGarland Ping. Issues like this is what created bad opinion about Boost libraries in certain circles where performance is important :( |
|
I really don't want to have a bad attitude on this change, but I'm not convinced it's the best way -- force inline seems like a non-standard hack -- I'd like to get out of that business. See my comment in the other issue about constexpr seems like that's the correct approach. Oh, I could really care less about what the performance oriented folks want -- I work for free on my spare time -- which means not on anyone else's timeline, sorry. If people really want performance they are free to hack the code as you have and otherwise. |
|
I'm sorry if I made it look like I expect free work from you, that's not the case. My point was that these issues are important to a certain subset of developers. Regarding constexpr, I agree that it would be the best solution. Unfortunately it does not seem to be enough in this case. I marked all functions involved in the constructor code path as I believe that if there turn out to be no better alternatives, BOOST_FORCEINLINE would be justified to apply in this case. It speeds up date_time library as a whole several times. There are already more than 2 thousands uses of BOOST_FORCEINLINE throughout the boost libraries. The date_time library itself already uses plenty of per-compiler code paths. |
|
Of course you want me to do work :) I was only triggered by: "Issues like this is what created bad opinion...". I'm sure no matter what I did someone won't be happy. Some have moved to Howard's library bc the fundamental design (no special values) makes things faster. Do you have the changes with the constexpr on a branch somewhere - I'd be curious to see that. Also, you're testing mostly with g++7.4? Presumably with at least std=c++14? |
|
Quite interesting to see the effect on code coverage from that one change... |
|
PR #214 is a better solution. |
Fixes #121
A small overview of benefits and potential downsides is extracted from #121 below:
This compiler workaround would improve the code size and speed for the majority of user workloads that are affected.
The only possibility this could be a regression would be in rarely executed call sites that pass non-constant value of special_values directly or indirectly to boost::gregorian::date::date. However, forcing inlining in such a case would just inflate code size a little bit and I suspect that the library user would still save code size overall due to significant reductions of code size around calls to default constructor of ptime and related types.
In my case the performance of whole application has been improved by around 5%.