Re: Remove redundant strlen call in ReplicationSlotValidateName

Em sex., 16 de jul. de 2021 às 13:18, Ranier Vilela <ranier(dot)vf(at)gmail(dot)com>
escreveu:

> Em sex., 16 de jul. de 2021 às 12:37, Alvaro Herrera <
> alvherre(at)alvh(dot)no-ip(dot)org> escreveu:
>
>> On 2021-Jul-16, David Rowley wrote:
>>
>> > On Fri, 16 Jul 2021 at 20:35, Japin Li <japinli(at)hotmail(dot)com> wrote:
>> > > > When I fix a bug about ALTER SUBSCRIPTION ... SET (slot_name) [1],
>> Ranier Vilela
>> > > > finds that ReplicationSlotValidateName() has redundant strlen()
>> call, Since it's
>> > > > not related to that problem, so I start a new thread to discuss it.
>> >
>> > I think this is a waste of time. The first strlen() call is just
>> > checking for an empty string. I imagine all compilers would just
>> > optimise that to checking if the first char is '\0';
>>
>> I could find the following idioms
>>
>> 95 times: var[0] == '\0'
>> 146 times: *var == '\0'
>> 35 times: strlen(var) == 0
>>
>> Resp.
>> git grep "[a-zA-Z_]*\[0\] == '\\\\0"
>> git grep "\*[a-zA-Z_]* == '\\\\0"
>> git grep "strlen([^)]*) == 0"
>>
>> See https://postgr.es/m/13847.1587332283@sss.pgh.pa.us about replacing
>> strlen with a check on first byte being zero. So still not Ranier's
>> patch, but rather the attached. I doubt this change is worth committing
>> on its own, though, since performance-wise it doesn't matter at all; if
>> somebody were to make it so that all "strlen(foo) == 0" occurrences were
>> changed to use the test on byte 0, that could be said to be establishing
>> a consistent style, which might be more pallatable.
>>
> Yeah, can be considered a refactoring.
>
> IMHO not in C is free.
> I do not think that this will improve 1% generally, but some function can
> gain.
>
> Another example I can mention is this little Lua code, in which I made
> comparisons between the generated asms, made some time ago.
>
> p[0] = luaS_newlstr(L, str, strlen(str));
>
> with strlen (msvc 64 bits):
> inc r8
> cmp BYTE PTR [r11+r8], 0
> jne SHORT $LL19(at)luaS_new
> mov rdx, r11
> mov rcx, rdi
> call luaS_newlstr
>
> without strlen (msvc 64 bits):
> mov r8, rsi
> mov rdx, r11
> mov QWORD PTR [rbx+8], rax
> mov rcx, rdi
> call luaS_newlstr
>
> Of course, that doesn't mean anything about Postgres, but that I'm talking
> about using strlen.
> Clearly I can see that usage is not always free.
>
> If have some interest in actually changing that in Postgres, I can prepare
> a patch,
> where static analyzers claim it's performance loss.
>
I did the patch, but to my surprise, the results weren't so good.
Despite that claiming a tiny improvement in performance, I didn't expect
any slowdown.
I put a counter in pg_regress.c, summing the results of each test and did
it three times for HEAD and for the patch.
Some tests were better, but others were bad.
Tests comments per example, show 180%, combocid 174%, dbize 165%, xmlmap
136%, lock 134%.

HEAD 1 HEAD 2 HEAD 3 HEAD avg
patch strlen1 patch strlen2 patch strlen3 patch avg

tablespace 326 328 339 331
321 320 335 325,333333333333 101,74%
boolean 63 48 62 57,6666666666667
63 39 62 54,6666666666667 105,49%
char 47 25 24 32
33 21 25 26,3333333333333 121,52%
name 30 33 26 29,6666666666667
34 26 62 40,6666666666667 72,95%
varchar 43 47 26 38,6666666666667
65 31 55 50,3333333333333 76,82%
text 51 43 53 49
37 57 54 49,3333333333333 99,32%
int2 29 66 46 47
57 58 38 51 92,16%
int4 55 35 46 45,3333333333333
51 54 70 58,3333333333333 77,71%
int8 103 85 111 99,6666666666667
79 96 91 88,6666666666667 112,41%
oid 70 85 71 75,3333333333333
45 64 80 63 119,58%
float4 68 74 86 76
84 68 49 67 113,43%
float8 98 87 109 98
95 98 88 93,6666666666667 104,63%
bit 99 84 91 91,3333333333333
108 96 92 98,6666666666667 92,57%
numeric 414 392 393 399,666666666667
379 399 411 396,333333333333 100,84%
txid 82 84 63 76,3333333333333
71 79 56 68,6666666666667 111,17%
uuid 48 96 76 73,3333333333333
79 76 67 74 99,10%
enum 101 125 102 109,333333333333
113 117 113 114,333333333333 95,63%
money 81 77 79 79
45 66 77 62,6666666666667 126,06%
rangetypes 404 411 415 410
426 404 421 417 98,32%
pg_lsn 60 63 69 64
69 57 51 59 108,47%
regproc 69 52 60 60,3333333333333
79 57 76 70,6666666666667 85,38%
strings 145 152 139 145,333333333333
170 138 136 148 98,20%
numerology 46 35 31 37,3333333333333
34 35 33 34 109,80%
point 45 37 45 42,3333333333333
52 38 76 55,3333333333333 76,51%
lseg 25 15 19 19,6666666666667
33 37 19 29,6666666666667 66,29%
line 31 30 24 28,3333333333333
20 24 32 25,3333333333333 111,84%
box 259 254 266 259,666666666667
255 307 305 289 89,85%
path 25 20 52 32,3333333333333
35 57 25 39 82,91%
polygon 274 269 271 271,333333333333
247 258 315 273,333333333333 99,27%
circle 35 52 55 47,3333333333333
43 33 51 42,3333333333333 111,81%
date 108 82 90 93,3333333333333
106 101 104 103,666666666667 90,03%
time 67 62 39 56
69 34 36 46,3333333333333 120,86%
timetz 38 62 41 47
36 75 33 48 97,92%
timestamp 416 412 425 417,666666666667
451 431 357 413 101,13%
timestamptz 479 468 474 473,666666666667
503 461 411 458,333333333333 103,35%
interval 81 104 93 92,6666666666667
69 96 79 81,3333333333333 113,93%
inet 72 74 108 84,6666666666667
83 80 85 82,6666666666667 102,42%
macaddr 37 61 43 47
59 74 72 68,3333333333333 68,78%
macaddr8 54 61 64 59,6666666666667
39 78 78 65 91,79%
multirangetypes 278 271 290 279,666666666667
290 333 265 296 94,48%
create_function_0 66 37 59 54
48 31 39 39,3333333333333 137,29%
geometry 153 136 133 140,666666666667
131 137 148 138,666666666667 101,44%
horology 102 132 98 110,666666666667
108 101 97 102 108,50%
tstypes 81 69 60 70
86 56 54 65,3333333333333 107,14%
regex 498 515 517 510
499 486 510 498,333333333333 102,34%
type_sanity 153 132 134 139,666666666667
161 130 144 145 96,32%
opr_sanity 389 377 381 382,333333333333
387 387 363 379 100,88%
misc_sanity 33 37 37 35,6666666666667
47 34 38 39,6666666666667 89,92%
comments 19 56 35 36,6666666666667
30 17 14 20,3333333333333 180,33%
expressions 62 44 67 57,6666666666667
52 40 69 53,6666666666667 107,45%
unicode 49 52 21 40,6666666666667
53 46 17 38,6666666666667 105,17%
xid 46 45 40 43,6666666666667
83 81 38 67,3333333333333 64,85%
mvcc 98 107 111 105,333333333333
95 116 94 101,666666666667 103,61%
create_function_1 9 10 10 9,66666666666667
10 10 9 9,66666666666667 100,00%
create_type 29 28 30 29
28 29 29 28,6666666666667 101,16%
create_table 365 364 364 364,333333333333
365 363 360 362,666666666667 100,46%
create_function_2 13 12 12 12,3333333333333
13 12 12 12,3333333333333 100,00%
copy 461 450 458 456,333333333333
463 454 450 455,666666666667 100,15%
copyselect 29 29 32 30
30 27 29 28,6666666666667 104,65%
copydml 35 35 35 35
37 35 33 35 100,00%
insert 303 309 338 316,666666666667
305 287 305 299 105,91%
insert_conflict 142 141 176 153
150 137 154 147 104,08%
create_misc 94 93 96 94,3333333333333
100 94 114 102,666666666667 91,88%
create_operator 24 27 22 24,3333333333333
28 26 24 26 93,59%
create_procedure 62 64 65 63,6666666666667
73 63 82 72,6666666666667 87,61%
create_index 880 933 866 893
896 864 904 888 100,56%
create_index_spgist 582 512 574 556
598 588 515 567 98,06%
create_view 341 250 329 306,666666666667
286 334 292 304 100,88%
index_including 183 207 204 198
215 143 163 173,666666666667 114,01%
index_including_gist 307 362 364 344,333333333333
413 260 278 317 108,62%
create_aggregate 62 69 90 73,6666666666667
60 100 107 89 82,77%
create_function_3 146 162 173 160,333333333333
151 144 181 158,666666666667 101,05%
create_cast 26 29 22 25,6666666666667
24 18 71 37,6666666666667 68,14%
constraints 210 218 216 214,666666666667
239 227 195 220,333333333333 97,43%
triggers 856 920 882 886
889 871 861 873,666666666667 101,41%
select 103 133 101 112,333333333333
92 133 140 121,666666666667 92,33%
inherit 615 627 671 637,666666666667
642 646 616 634,666666666667 100,47%
typed_table 146 129 131 135,333333333333
115 121 116 117,333333333333 115,34%
vacuum 494 417 455 455,333333333333
442 451 402 431,666666666667 105,48%
drop_if_exists 115 84 83 94
138 63 57 86 109,30%
updatable_views 645 630 678 651
635 644 647 642 101,40%
roleattributes 119 95 80 98
105 134 90 109,666666666667 89,36%
create_am 156 114 126 132
155 167 191 171 77,19%
hash_func 83 52 61 65,3333333333333
50 50 91 63,6666666666667 102,62%
errors 54 80 63 65,6666666666667
91 33 41 55 119,39%
infinite_recurse 312 321 361 331,333333333333
329 341 401 357 92,81%
sanity_check 136 134 134 134,666666666667
135 134 134 134,333333333333 100,25%
select_into 91 139 66 98,6666666666667
81 118 78 92,3333333333333 106,86%
select_distinct 228 202 181 203,666666666667
242 238 214 231,333333333333 88,04%
select_distinct_on 27 38 60 41,6666666666667
44 36 24 34,6666666666667 120,19%
select_implicit 77 48 47 57,3333333333333
44 51 94 63 91,01%
select_having 38 48 75 53,6666666666667
50 64 39 51 105,23%
subselect 274 297 305 292
316 351 310 325,666666666667 89,66%
union 308 285 289 294
351 289 281 307 95,77%
case 53 137 47 79
81 56 149 95,3333333333333 82,87%
join 685 718 736 713
690 678 793 720,333333333333 98,98%
aggregates 706 675 697 692,666666666667
802 680 728 736,666666666667 94,03%
transactions 285 182 273 246,666666666667
159 247 251 219 112,63%
random 44 67 46 52,3333333333333
43 82 50 58,3333333333333 89,71%
portals 264 265 189 239,333333333333
263 247 225 245 97,69%
arrays 379 355 358 364
352 398 339 363 100,28%
btree_index 1550 1532 1476 1519,33333333333
1708 1565 1557 1610 94,37%
hash_index 436 480 477 464,333333333333
464 447 440 450,333333333333 103,11%
update 468 464 464 465,333333333333
522 464 472 486 95,75%
delete 112 43 48 67,6666666666667
101 89 41 77 87,88%
namespace 96 50 56 67,3333333333333
136 52 49 79 85,23%
prepared_xacts 185 109 160 151,333333333333
161 127 175 154,333333333333 98,06%
brin 1689 1719 1740 1716
1777 1671 1704 1717,33333333333 99,92%
gin 1166 1213 1139 1172,66666666667
1215 1243 1095 1184,33333333333 99,01%
gist 1052 1045 1073 1056,66666666667
1029 1072 1064 1055 100,16%
spgist 1013 1029 1094 1045,33333333333
1009 946 994 983 106,34%
privileges 876 863 904 881
924 942 1008 958 91,96%
init_privs 27 20 25 24
44 18 17 26,3333333333333 91,14%
security_label 43 49 63 51,6666666666667
89 46 170 101,666666666667 50,82%
collate 315 393 346 351,333333333333
319 222 341 294 119,50%
matview 671 699 663 677,666666666667
698 676 723 699 96,95%
lock 220 171 177 189,333333333333
112 132 179 141 134,28%
replica_identity 294 248 235 259
314 229 428 323,666666666667 80,02%
rowsecurity 869 888 832 863
853 919 933 901,666666666667 95,71%
object_address 244 210 202 218,666666666667
257 224 301 260,666666666667 83,89%
tablesample 154 164 194 170,666666666667
217 149 169 178,333333333333 95,70%
groupingsets 644 606 640 630
564 637 665 622 101,29%
drop_operator 98 125 55 92,6666666666667
54 72 76 67,3333333333333 137,62%
password 585 572 535 564
527 538 465 510 110,59%
identity 529 435 499 487,666666666667
504 571 453 509,333333333333 95,75%
generated 777 672 759 736
830 744 734 769,333333333333 95,67%
join_hash 1672 1706 1726 1701,33333333333
1761 1667 1691 1706,33333333333 99,71%
brin_bloom 97 97 131 108,333333333333
99 109 97 101,666666666667 106,56%
brin_multi 236 234 269 246,333333333333
245 236 226 235,666666666667 104,53%
create_table_like 281 277 257 271,666666666667
283 277 280 280 97,02%
alter_generic 124 116 126 122
119 131 137 129 94,57%
alter_operator 33 28 31 30,6666666666667
61 36 50 49 62,59%
misc 161 172 157 163,333333333333
176 160 167 167,666666666667 97,42%
async 19 13 40 24
33 60 22 38,3333333333333 62,61%
dbsize 73 31 22 42
24 19 33 25,3333333333333 165,79%
misc_functions 84 79 84 82,3333333333333
94 89 92 91,6666666666667 89,82%
sysviews 97 108 86 97
91 88 92 90,3333333333333 107,38%
tsrf 69 72 80 73,6666666666667
60 96 84 80 92,08%
tid 84 37 38 53
47 73 57 59 89,83%
tidscan 75 61 81 72,3333333333333
98 77 96 90,3333333333333 80,07%
tidrangescan 83 49 76 69,3333333333333
47 69 63 59,6666666666667 116,20%
collate.icu.utf8 30 37 29 32
16 60 27 34,3333333333333 93,20%
incremental_sort 174 176 163 171
160 164 179 167,666666666667 101,99%
rules 303 383 315 333,666666666667
385 376 341 367,333333333333 90,83%
psql 253 315 253 273,666666666667
329 243 297 289,666666666667 94,48%
psql_crosstab 35 31 30 32
49 33 33 38,3333333333333 83,48%
amutils 18 20 18 18,6666666666667
34 20 18 24 77,78%
stats_ext 1530 1568 1572 1556,66666666667
1576 1605 1571 1584 98,27%
collate.linux.utf8 10 11 11 10,6666666666667
17 10 11 12,6666666666667 84,21%
select_parallel 786 786 797 789,666666666667
797 807 826 810 97,49%
write_parallel 78 80 80 79,3333333333333
79 79 84 80,6666666666667 98,35%
publication 76 83 81 80
74 75 74 74,3333333333333 107,62%
subscription 41 48 48 45,6666666666667
40 43 43 42 108,73%
select_views 201 222 199 207,333333333333
188 205 204 199 104,19%
portals_p2 57 38 41 45,3333333333333
35 44 34 37,6666666666667 120,35%
foreign_key 969 955 965 963
925 971 947 947,666666666667 101,62%
cluster 407 400 408 405
417 439 398 418 96,89%
dependency 107 212 112 143,666666666667
178 147 164 163 88,14%
guc 101 184 168 151
177 181 161 173 87,28%
bitmapops 448 443 429 440
444 460 446 450 97,78%
combocid 156 144 150 150
46 162 50 86 174,42%
tsearch 396 443 424 421
404 434 415 417,666666666667 100,80%
tsdicts 137 170 166 157,666666666667
98 157 130 128,333333333333 122,86%
foreign_data 633 637 636 635,333333333333
647 648 629 641,333333333333 99,06%
window 374 428 391 397,666666666667
353 374 355 360,666666666667 110,26%
xmlmap 108 51 113 90,6666666666667
85 45 69 66,3333333333333 136,68%
functional_deps 177 169 125 157
145 162 144 150,333333333333 104,43%
advisory_lock 71 68 79 72,6666666666667
72 57 86 71,6666666666667 101,40%
indirect_toast 541 522 554 539
567 627 596 596,666666666667 90,34%
equivclass 123 197 118 146
184 178 165 175,666666666667 83,11%
json 106 105 111 107,333333333333
116 108 110 111,333333333333 96,41%
jsonb 246 253 256 251,666666666667
262 253 256 257 97,92%
json_encoding 17 16 16 16,3333333333333
17 15 19 17 96,08%
jsonpath 31 32 31 31,3333333333333
32 32 32 32 97,92%
jsonpath_encoding 15 14 14 14,3333333333333
15 15 14 14,6666666666667 97,73%
jsonb_jsonpath 81 81 84 82
83 82 84 83 98,80%
plancache 107 103 145 118,333333333333
131 101 120 117,333333333333 100,85%
limit 186 130 127 147,666666666667
138 167 138 147,666666666667 100,00%
plpgsql 725 713 739 725,666666666667
726 699 722 715,666666666667 101,40%
copy2 324 227 187 246
226 137 226 196,333333333333 125,30%
temp 215 236 251 234
208 178 190 192 121,88%
domain 380 368 391 379,666666666667
371 351 351 357,666666666667 106,15%
rangefuncs 379 336 371 362
354 373 359 362 100,00%
prepare 164 175 131 156,666666666667
87 179 149 138,333333333333 113,25%
conversion 190 113 198 167
186 259 221 222 75,23%
truncate 349 362 365 358,666666666667
285 342 305 310,666666666667 115,45%
alter_table 1419 1415 1363 1399
1406 1409 1402 1405,66666666667 99,53%
sequence 260 227 312 266,333333333333
236 264 212 237,333333333333 112,22%
polymorphism 298 341 357 332
342 341 323 335,333333333333 99,01%
rowtypes 291 266 284 280,333333333333
281 254 309 281,333333333333 99,64%
returning 146 92 64 100,666666666667
106 243 108 152,333333333333 66,08%
largeobject 365 447 415 409
426 494 426 448,666666666667 91,16%
with 385 329 360 358
345 286 349 326,666666666667 109,59%
xml 160 222 217 199,666666666667
289 253 228 256,666666666667 77,79%
partition_join 865 861 861 862,333333333333
816 924 872 870,666666666667 99,04%
partition_prune 783 719 791 764,333333333333
758 758 781 765,666666666667 99,83%
reloptions 96 84 78 86
74 74 87 78,3333333333333 109,79%
hash_part 51 43 39 44,3333333333333
46 57 58 53,6666666666667 82,61%
indexing 702 707 685 698
737 714 739 730 95,62%
partition_aggregate 949 939 884 924
960 957 915 944 97,88%
partition_info 96 111 77 94,6666666666667
111 83 104 99,3333333333333 95,30%
tuplesort 1359 1339 1387 1361,66666666667
1404 1381 1359 1381,33333333333 98,58%
explain 97 101 82 93,3333333333333
102 102 74 92,6666666666667 100,72%
compression 164 157 162 161
154 149 156 153 105,23%
memoize 90 95 87 90,6666666666667
80 75 84 79,6666666666667 113,81%
event_trigger 126 126 135 129
127 111 112 116,666666666667 110,57%
oidjoins 257 240 252 249,666666666667
245 243 245 244,333333333333 102,18%
fast_default 146 145 145 145,333333333333
148 145 162 151,666666666667 95,82%
stats 615 611 611 612,333333333333
621 613 611 615 99,57%

So I'm posting the patch here, merely as an illustration of my findings.
Perhaps someone with a better understanding of the process of translating C
to asm can have an explanation.
Is it worth it to change only where there has been improvement?

regards,
Ranier Vilela