Patch 4: removed unnecessary calls to strlen() in Curl_compareheader by using a new CONSTLEN macro for our string literals

[HenrikHolst]

removed unnecessary calls to strlen() in Curl_compareheader by using a new CONSTLEN macro for our string literals

[jay]

Ref: https://curl.se/mail/lib-2022-02/0042.html

#define CONSTLEN(x) x,sizeof(x)-1

I'm not a fan of this style because it hides a parameter behind a macro. I'd rather we just leave the strlens but judging by that e-mail thread I seem to be in the minority.

[HenrikHolst]

Well the problem is that in order to keep the strlen and have the compiler optimize it away and not hide a parameter behind a macro we must call functions like this: Curl_compareheader(ptr, "Expect:", strlen ("Expect:"), "100-continue", strlen ("100-continue")); AFAIK OpenSSL learned that lesson some years ago the hard way that this was error prone, aka one of the strings where changed at some point in time but not the other so the length given was off and they had a CVE on their hand. The other solution is to keep the strlen inside the Curl_compareheader function and just take the overhead, there have been claims that some compiler might optimize away these but since they are present in the trace I would say that the compiler does not optimize away these. Now the overhead that we speak of here is of course small compared with the actual time it takes to download anything off the network and for a single fetch this might be a completely undetectable change, but if you manage several connections in a busy multi interface this change should lead to some µs less latency for each setup and those can easily add up. But then a clean easily maintained code base is also of value so the question comes down to how much it pollutes the readability of the source vs how much the runtime benefits are. /HH Den fre 4 feb. 2022 kl 22:17 skrev Jay Satiro ***@***.***>:

…

Ref: https://curl.se/mail/lib-2022-02/0042.html #define CONSTLEN(x) x,sizeof(x)-1 I'm not a fan of this style because it hides a parameter behind a macro. I'd rather we just leave the strlens but judging by that e-mail thread I seem to be in the minority. — Reply to this email directly, view it on GitHub <#8391 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACM6IBWRZGKEI5M6UQGGL5DUZQ66NANCNFSM5NSV42UQ> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you authored the thread.Message ID: ***@***.***>

[dfandrich]

On Fri, Feb 04, 2022 at 01:17:29PM -0800, Jay Satiro wrote: I'm not a fan of this style because it hides a parameter behind a macro. I'd rather we just leave the strlens but judging by that e-mail thread I seem to be in the minority.

I'm on your side. I'd first like to see evidence that 1) there are compilers in significant use that *don't* optimize strlen already, 2) that such a macro actually makes a noticeable difference in performance, and 3) that the decrease in readability in all platforms is worth it.

[HenrikHolst]

Den fre 4 feb. 2022 kl 22:39 skrev Dan Fandrich ***@***.***>:

On Fri, Feb 04, 2022 at 01:17:29PM -0800, Jay Satiro wrote: > I'm not a fan of this style because it hides a parameter behind a macro. I'd > rather we just leave the strlens but judging by that e-mail thread I seem to be > in the minority. I'm on your side. I'd first like to see evidence that 1) there are compilers in significant use that *don't* optimize strlen already, 2) that such a macro actually makes a noticeable difference in performance, and 3) that the decrease in readability in all platforms is worth it.

1) no this does not happen and I don't really know where this comes from. For this to be true then the compiler will have to analyse every single source across file boundaries to determine that all sources are from string literals. But instead of pure theory, here is with GCC v9.3.0 (the default in Ubuntu 20.04LTS) : With the patch: ***@***.***:~/utveckling/repositories$ uftrace -- ./curl/src/curl --silent -Ls 'https://curl.se/' -o /dev/null | grep strlen | wc 152 760 8900 Without the patch: ***@***.***:~/utveckling/repositories$ uftrace -- ./curl2/src/curl --silent -Ls 'https://curl.se/' -o /dev/null | grep strlen | wc 173 865 10181 Where this is weak is of course in #2 since as you can see above there where only 21 calls to strlen() removed by the patch when fetching the curl.se , for the google.com test it was slightly better (129 calls to strlen removed). But, and this is with a huge maybe, this is from patching just Curl_compareheader(), there might be other string literals in the source where the length is used that could be reduced later. No guarantees though. /HH

…

— Reply to this email directly, view it on GitHub <#8391 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACM6IBU4PX5D2KVXT7SR7FLUZRBSTANCNFSM5NSV42UQ> . Triage notifications on the go with GitHub Mobile for iOS <https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675> or Android <https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>. You are receiving this because you authored the thread.Message ID: ***@***.***>

[bagder]

I'm not a fan of this style because it hides a parameter behind a macro. I'd rather we just leave the strlens but judging by that e-mail thread I seem to be in the minority.

I'm generally against hiding code behind macros, but I find @HenrikHolst's argument about not having to repeat the string convincing. I can't think of a better way to do this change than what is being proposed here, so I'm a 👍 on it.

[HenrikHolst]

Not sure if this is the proper channel to continue this discussion in this manner but if/when this patch gets accepted then I have also identified a few other areas where the CONSTLEN macro will be useful.

One such case is many calls like this:
result = Curl_dyn_add(&stream->header_recvbuf, "\r\n");

That should either be changed to:
result = Curl_dyn_addn(&stream->header_recvbuf, "\r\n", 2);

Or my preferred way:
result = Curl_dyn_addn(&stream->header_recvbuf, STRCONST("\r\n"));

And I would prefer to do it that way (but I can do it the ,2 way if people tell me to) so that if the string is changed we are sure that the length is also changed.

And before the optimizing compiler comes up again, I have compiled curl with -O3 and still a trace gives:

            [226428] |                       Curl_http_bodysend() {
            [226428] |                         Curl_dyn_add() {
   0.180 us [226428] |                           strlen("^M\n") = 2;
            [226428] |                           dyn_nappend() {
   0.190 us [226428] |                             memcpy(0x557fb893f2cf, &__PRETTY_FUNCTION__.11279, 2);
   0.471 us [226428] |                           } /* dyn_nappend */
   1.143 us [226428] |                         } /* Curl_dyn_add */

So at least GCC does not magically switch to use the CUrl_dyn_addn function instead. We can also see the cost here, it's 0.180 µs on my Ryzen 1600X even for such a small string so even if we reduce the amount by say 10 such small strlen:s we are still only talking about saving ~2µs so the savings are not enormous, but they are there.

[bagder]

result = Curl_dyn_addn(&stream->header_recvbuf, STRCONST("\r\n"));

I could live with this approach. This would just become a standard internal macro we would learn to live with and use. I think STRCONST() might be a better name than CONSTLEN().

[HenrikHolst]

I agree that STRCONST is the better name so I've changed it in the pull request.

[bagder]

Thanks!