Skip to content
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

functypes.h: define the arg types for send and recv #9592

Closed
wants to merge 2 commits into from

Conversation

bagder
Copy link
Member

@bagder bagder commented Sep 26, 2022

Replaces the brute-force (slow) checks done in configure and cmake.

Unit test 1662 verifies that the new defines have the same values as the old.

@bagder bagder added the build label Sep 26, 2022
@bagder

This comment was marked as outdated.

@vszakats
Copy link
Member

vszakats commented Sep 26, 2022

There seem to be support (apparently on the Windows platform) for an alternate, BSD-style TCP/IP API called lwIP, enabled via USE_LWIPSOCK. Some of its types are different. Not sure if this is actively used, or even universally supported in curl.

@bagder
Copy link
Member Author

bagder commented Sep 26, 2022

We have some partial support for lwip. I don't know how complete that is.

I plan to make this way of setting type types generic enough to work with that if someone wants to get a build like that going anyway.

I will let the dedicated config-[platform].h files define the types like today, remove the checks from configure/cmake and then have "fallback defines" in functypes.h that will default to the POSIX types - unless other types are defined.

@bagder bagder changed the title functypes.h: define the arg types for send, recv and select functypes.h: define the arg types for send and recv Sep 26, 2022
This header is for providing the argument types for recv() and send()
when built to not use a dedicated config-[platfor].h file.

Remove the slow brute-force checks from configure and cmake.

This change also removes the use of the types for select, as they were
not used in code.

Closes #9592
@bagder bagder marked this pull request as ready for review Sep 26, 2022
@bagder
Copy link
Member Author

bagder commented Sep 26, 2022

On my Linux host, running configure with and without this patch shows no particular speed difference and individual timing differences between invokes is larger (my default build configure line takes ~18 seconds to complete).

Presumably this is because after @vszakats's recent optimization it just a few compiles less and they are very fast here.

@vszakats
Copy link
Member

vszakats commented Sep 26, 2022

It should now also be faster when using CMake on non-Windows. That one was previously favoring Windows, and the "unlucky" case was Linux and other platforms.

Either way, it feels much better just to look at the new solution. It will also surely save some energy for all.

I run more curl-for-win tests (without this PR), and CMake has a 20% overhead compared to Makefile.m32, autotools has 30%. (being 42% before, and even 50% a few months ago.) One reason is that libcurl is compiled twice with cmake/autotools, plus the still quite resource intensive auto-detection phases (also run twice with both cmake/autotools).

One (off-topic) way to improve the above, if libcurl objects would be compiled just once and being re-used for both the dynamic and static libs. Technically doable, but no idea how with cmake/autotools [*]. There is also a built-in second compilation pass of libcurl (named libcurlu) when tests are enabled in autotools.

[*] autotools likely can do this, but with pitfalls, like no option (that I could discover) to chose which lib flavor to link with curl.

@bagder
Copy link
Member Author

bagder commented Sep 26, 2022

Either way, it feels much better just to look at the new solution. It will also surely save some energy for all.

I too think this feels like a much cleaner and nicer solution. It will however probably cause build breakages in the short-term on some fringe platforms. They should be easy to fix though.

@vszakats
Copy link
Member

vszakats commented Sep 27, 2022

/off I've seen a suggestion on the mailing list to use caching with autotools and cmake. For CI and curl-for-win these don't seem to be useful by default and have many pitfalls, but with manual/fragile uncaching + hacks, they helped reducing curl-for-win build times by a little bit. Estimated 2 minutes total with CMake (-5%), and ~45 seconds with autotools (-1–2%). I'm guessing the reason for the mild results with autotools is that it misses caching quite a few detection results. [1] [2]

@bagder
Copy link
Member Author

bagder commented Sep 28, 2022

Yes, we can/should still work on improved caching.

@bagder bagder closed this in eb33ccd Sep 28, 2022
@bagder bagder deleted the bagder/functypes branch Sep 28, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants