An unlikely() experiment
Many of you might know of GCC's __builtin_expect() and that it allows us to
tell the compiler which code branches are more likely to occur. The
documentation on this does warn that normally it's a bad idea to use this
"as programmers are notoriously bad at predicting how their programs
actually perform" [1]https://gcc.gnu.org/onlinedocs/gcc/Other-Builtins.html. So I choose to ignore that, and give it a try
anyway... elog(ERROR) for example, surely that branch should always be in a
cold path? ... ereport() too perhaps?
Anyway, I knocked up a patch which went an added an errcheck() macro which
is defined the same as unlikely() is, and I stuck these around just the
"if" conditions for tests before elog(ERROR) calls, such as:
if (errcheck(newoff == InvalidOffsetNumber))
elog(ERROR, "failed to add BRIN tuple to new page");
gcc version 5.2.1 on i7-4712HQ
pgbench -M prepared -T 60 -S
Master: 12246 tps
Patched 13132 tsp (107%)
pgbench -M prepared -T 60 -S -c 8 -j 8
Master: 122982 tps
Patched: 129318 tps (105%)
I thought this was quite interesting.
Ok, so perhaps it's not that likely that we'd want to litter these
errcheck()s around everywhere, so I added a bit more logging as I thought
it's probably just a handful of calls that are making this improvement. I
changed the macro to call a function to log the __FILE__ and __LINE__, then
I loaded the results into Postgres, aggregated by file and line and sorted
by count(*) desc. Here's a sample of them (against bbbd807):
file | line | count
------------------+------+--------
fmgr.c | 1326 | 158756
mcxt.c | 902 | 147184
mcxt.c | 759 | 113162
lwlock.c | 1545 | 67585
lwlock.c | 938 | 67578
stringinfo.c | 253 | 55364
mcxt.c | 831 | 47984
fmgr.c | 1030 | 36271
syscache.c | 978 | 28182
dynahash.c | 981 | 22721
dynahash.c | 1665 | 19994
mcxt.c | 931 | 18594
Perhaps it would be worth doing something with the hottest ones maybe?
Alternatively, if there was some way to mark the path as cold from within
the path, rather than from the if() condition before the path, then we
could perhaps do something in the elog() macro instead. I just couldn't
figure out a way to do this.
Does anyone have any thoughts on this?
[1]: https://gcc.gnu.org/onlinedocs/gcc/Other-Builtins.html
--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
Hi,
On 2015-12-20 02:49:13 +1300, David Rowley wrote:
Many of you might know of GCC's __builtin_expect() and that it allows us to
tell the compiler which code branches are more likely to occur.
It also tells the reader that, which I find also rather helpful.
The documentation on this does warn that normally it's a bad idea to
use this "as programmers are notoriously bad at predicting how their
programs actually perform" [1].
I think we indeed have to careful to not add this to 40/60 type
branches. But 99/1 or so paths are ok.
So I choose to ignore that, and give
it a try anyway... elog(ERROR) for example, surely that branch should
always be in a cold path? ... ereport() too perhaps?
Hm, not exactly what you were thinking of, but it'd definitely be
interesting to add unlikely annotations to debug elogs, in some
automated manner (__builtin_choose_expr?). I've previously hacked
elog/ereport to only support >= ERROR, and that resulted in a speedup,
removing a log of elog(DEBUG*) triggered jumps.
Anyway, I knocked up a patch which went an added an errcheck() macro which
is defined the same as unlikely() is, and I stuck these around just the
"if" conditions for tests before elog(ERROR) calls, such as:
Personally I'd rather go with a plain unlikely() - I don't think
errcheck as a name really buys us that much, and it's a bit restrictive.
gcc version 5.2.1 on i7-4712HQ
pgbench -M prepared -T 60 -S
Master: 12246 tps
Patched 13132 tsp (107%)pgbench -M prepared -T 60 -S -c 8 -j 8
Master: 122982 tps
Patched: 129318 tps (105%)
Not bad at all.
Ok, so perhaps it's not that likely that we'd want to litter these
errcheck()s around everywhere, so I added a bit more logging as I thought
it's probably just a handful of calls that are making this improvement. I
changed the macro to call a function to log the __FILE__ and __LINE__, then
I loaded the results into Postgres, aggregated by file and line and sorted
by count(*) desc. Here's a sample of them (against bbbd807):file | line | count
------------------+------+--------
fmgr.c | 1326 | 158756
mcxt.c | 902 | 147184
mcxt.c | 759 | 113162
lwlock.c | 1545 | 67585
lwlock.c | 938 | 67578
stringinfo.c | 253 | 55364
mcxt.c | 831 | 47984
fmgr.c | 1030 | 36271
syscache.c | 978 | 28182
dynahash.c | 981 | 22721
dynahash.c | 1665 | 19994
mcxt.c | 931 | 18594
To make sure I understand correctly: Before doing that you manually
added the errcheck()'s manually? At each elog(ERROR) callsite?
Perhaps it would be worth doing something with the hottest ones maybe?
One way to do this would be to add elog_on() / ereport_on() macros,
directly containing the error message. Like
#define elog_on(cond, elevel, ...) \
do { \
if (unlikely(cond)) \
{ \
elog(elevel, __VA_ARGS__) \
} \
} while(0)
Greetings,
Andres
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On 20 December 2015 at 03:06, Andres Freund <andres@anarazel.de> wrote:
On 2015-12-20 02:49:13 +1300, David Rowley wrote:
So I choose to ignore that, and give
it a try anyway... elog(ERROR) for example, surely that branch should
always be in a cold path? ... ereport() too perhaps?
Hm, not exactly what you were thinking of, but it'd definitely be
interesting to add unlikely annotations to debug elogs, in some
automated manner (__builtin_choose_expr?). I've previously hacked
elog/ereport to only support >= ERROR, and that resulted in a speedup,
removing a log of elog(DEBUG*) triggered jumps.
Good thinking. It would be interesting to see benchmarks with the hacked
version of elog()
To make sure I understand correctly: Before doing that you manually
added the errcheck()'s manually? At each elog(ERROR) callsite?
Yeah manually at most call sites. Although I did nothing in cases like;
if (somethinggood)
dosomethinggood();
else
elog(ERROR, ...);
Quite possibly in that case we could use likely(), but there's also cases
where elog() is in the default: in a switch(), or contained in an else in
an if/else if/else block.
I also left out errcheck() in places where the condition called a function
with some side affect. I did this as I also wanted to test a hard coded
false condition to see how much overhead remained. I did nothing with
ereport().
One way to do this would be to add elog_on() / ereport_on() macros,
directly containing the error message. Like
#define elog_on(cond, elevel, ...) \
do { \
if (unlikely(cond)) \
{ \
elog(elevel, __VA_ARGS__) \
} \
} while(0)
Interesting idea. Would you think that would be something we could do a
complete replace on, or are you thinking just for the hotter code paths?
I've attached the patch this time, it should apply cleanly to bbbd807. I'd
would be good to see some other performance tests on some other hardware.
--
David Rowley http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
Attachments:
errcheck1.patch.bz2application/x-bzip2; name=errcheck1.patch.bz2Download
BZh91AY&SY4!�"����{�����������a�=���d 4
m�h �����k@��]d: ���{����S���T}H�EA�v�/��OV�]8��Uv��x��:�@������x���w�=\��[�-Uu���^�}z���]6�]����L�*� `}}���� ��X`��e�U: � �`���@
�� /�@�=>��������� /yY�w���M��U��������v�7���!��n�]�|��������+YJt(�=���rS��:zPj����� G��xi���: � ����*[V��yH ��y�� s�������.������n����|���_J�@@:
4�� � � <uef�+c�9g�Y��L�S6W��V��Z�O����W6TR��"�s��$��nI{|����E��Ju����s�:�r���W���}|���7cc�g���������������zV�wsn� (C�:44�
� ���'��)df��f���5��sj����Dj�Na[��z�y�t�{��)���l|�{��}g�z���6H<��z�Q��.O^�wr��J���)���uCvZ�>�������os�����)R��J�u�Uu�7x�i!��;Z�8K�#n��:��{�T�HFX ��J�m����������W�:��^�{<|��y�2+��k����On������-���j�Q�W�hr��������UtJJ^�����J�N����A ]CXC�����V���1K�z�6y���������p����@4�k6��j�l��s�������w��<����J��owk�C�� �����d���{��3�����fz�����<�x��g��t���w���#��[raC��N����w*�CW-��E�PM�0���^���[]�B���]��XZ���.��n�uC*�"�l>��3�����n����^vu��{2����w���wj�� ���owl\�v'v��M����5�:���97��vn������_m�����+����c��)��n*;t��{�s�{m������pt=�����;^���g}�z��w8��-���BKl�h^� �y�vs��h8��#G�����E�{�#��$�4 M�F��)��14��OS��S���S@�z��3Dm4@�� d $!!4����4�Jm�O�<i{T��zi�#OQ�@OT�����j�54 4�@��M� �$�� @bb�i�MO"h���L���z�Q$ @M���@M& �L�zi�i��6�@
�I��[X��2D����DD��'�[�D��fb�nKj�U�QS�mn�ZB�� �� JH!I�I�`��"Ff*�e-��R6`9�&4`�a@�&
���*�f)�{�U���;���o��-���������d� -
`1�� ����h��Ia���F��("m&�06�������6���,U�E�M�[������t�c�Xh��hmb�@3�$�-�(�!Z��� ZH���?��/�����u�d���&$��b�����-lZ���u{�����0�� ,�-�A�$��_���K�:u��R�5�u+��/z��s[���nK��H�����������M���v���r�����^����S52i��2A�3Y�L���y����/��;oLm&�&4��_�.��AD���:�/�MhO�I��h��%�i�
�#,�`4��$�f�����C%H�F0X���H�Q�E��,�Jk�0���
!J���FYL�jb�II�I��*6"�,���b�&Ll$�����&f��4h�����J�1�jiD����60H�ei�E6%,}���{J��vJ+�u�I7��^�>�f9��e��$����!��c&hDK%�h��k�������X����I
r��w M����s��"7C���6���Z�c&hD,���-&�! I�@hM$�b�"4Q�#bfT �i�(4E��
52*I��Q����2�e@��8����������.��u�n��jw��������.u��d�6�����w"(�����E[Y&d��w}w���G+��}��dU_�����%r#�Z4�<��lm1�m��i���1��P��#q���bdX�Z6Q�%�u)��QL�g�������{i����6����I�,������z���9����f# J����b�5*���Q��3��
���V����/��y���� ��o�b4RL#aBI�[&CX3)�S4�5�3H�0�B"�m���[-Z�Va�I�b��D1��SB6�ID�)�4�QF��X�IH����E&�d�4d�f��n���q�d��M�\
�I��wk�L�n�k��*{�H(SYA$�Q���KDT�3Q��P�If���S@��R$�E��4f���L)Idh�"jE"Q
d@�5*Fc#H`IP�KFi�f���n�I��7��e�X��d'����3���L�#l�1`������&����h��wRiP�P6U��Y*I~�K���L�������dJT�H�R�I1#i�l�f�T���!3Rh��S2h�eKl����[f���F�P�(��X�n�u�vm��R��s���K����u99]'.���I:��N�������B]Mb�5r��,��n�����M(N��gw];����a�����t�����T� ��E5*��E*[eeTU�6�5��b)C ��2��� �)
d6A������%��I�L��K$&�m1Ie����
�Q�������&I�l 2#A)Q,�5--,l�X�"��U%"���fM� �1X�6"R��$2Sd�B�4�L����2�s�]�m�&]u��p�$�K�u5.j�Kw[��n�sY�nn����q Li��u3����\RM��c7v��d����n�,�!��0�K�]wwuv�����]��.�u�wL�]�2���\%%&M�Zi)
�Jf)��Xd��!��~���I�`��_f�&$�B�l��i���$�����FS�_�7�MI���](�34�����R~m�6�ni(���0��U�k��s2B�R5*I#�4��L��fZM�!������jIL�iI�J��MF��,�-lV��W�yIBYDB��l`�N�#IFcfQk�����h��
m5�me�P��6���V�#&�i��CCAU4�|#����~�~��m��2��r�^���������l���q��kU�`l`�3N&�o��m��������{}����+Q4%�M�,q���}M6�dnv�n������CD|f��)n��&]^8�-s������m����x o�����G�GR(��iJH%{kj���y�YW���&kg��7�B�+��������O]������:�)0R�.�Sk`��6��� ���.JC��!����6��v���["))�H�����1��
}=���~}^&��&�D@�'��Hv{���-��A�1����>H�M|N7����7p�]�����3E��&�@��!�M��yI�s��;JI N���6�5�1�5&�����(�B��1�+����Q�N8��2]L&<���+������T�;�U�e����d���{�kx�;OG����f^��p��N��-���*����r����Bp/��$��Z������%�9A�z���uC�d1��&�y�0�m�}�(��,1�����>"�W��`6A,r@d�����+WSR��f����6��T��C� �O��HD`����Md���43ji��Wg~cJ�c��.a-�8�����M6�H��$�Vj�UV��cmf�f����������
1 SITm�-�b"��h��6*�E���QRQD�q ZpJ@�H,�t��f���:nE������CT��D"