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

save a bit of space with some structure packing #6483

Closed
wants to merge 1 commit into from

Conversation

@eriols
Copy link
Contributor

@eriols eriols commented Jan 18, 2021

This is an attempt at saving a bit of space by packing some structs (using pahole to find the holes) where it might make sense to do so without losing readability.
I.e., I tried to avoid separating fields that seem grouped together (like the cwd... fields in struct ftp_conn for instance).
Also abstained from touching fields behind conditional macros as that quickly can get complicated.

If this is at all useful (and I doubt it) I'm happy to squash; kept them separate for now to easy be able any unwanted ones.

@jay jay added the tidy-up label Jan 18, 2021
Copy link
Member

@jay jay left a comment

Looks reasonable, please squash

lib/ftp.h Outdated
/* newhost is the (allocated) IP addr or host name to connect the data
connection to */
unsigned short newport; /* connection to */
char *newhost; /* this is the pair to connect the DATA... */
Copy link
Member

@jay jay Jan 18, 2021

These comments should be reversed but IMO are not needed at all because of the comment block above it

Copy link
Contributor Author

@eriols eriols Jan 19, 2021

Thanks, removing the per-field comments.

@bagder
Copy link
Member

@bagder bagder commented Jan 19, 2021

Summed up, all these tiny changes add up of course and I don't mind saving this memory at basically no cost.

What's perhaps a little annoying is that the general idea to manually pack structs is to keep them in size order, in a large to small order but this bot only breaks that but it seems that we will add or change entries going forward that will alter this "optimized" state.

@eriols, you mentioned you worked with the pahole tool to figure these out. Can we come up with a way to run that tool automatically somehow to check/update the struct status?

unsigned int perm;
time_t time; /* always zero! */
Copy link
Member

@MarcelRaad MarcelRaad Jan 19, 2021

Is this one OK @bagder @jay ? Isn't this an ABI change in a public struct?

Copy link
Member

@bagder bagder Jan 19, 2021

Good catch, I didn't see that. Correct, we must not change this!

Copy link
Member

@jay jay Jan 19, 2021

Yes I missed that

Copy link
Contributor Author

@eriols eriols Jan 19, 2021

Thanks, dropping that one!

@@ -1905,6 +1906,7 @@ struct Curl_easy {
int actions[MAX_SOCKSPEREASYHANDLE]; /* action for each socket in
sockets[] */
int numsocks;
unsigned int magic; /* set to a CURLEASY_MAGIC_NUMBER */
Copy link
Member

@bagder bagder Jan 19, 2021

Spotting this change made me think and create #6484 instead

This is an attempt at saving a bit of space by packing some structs
(using pahole to find the holes) where it might make sense to do
so without losing readability.
I.e., I tried to avoid separating fields that seem grouped
together (like the cwd... fields in struct ftp_conn for instance).
Also abstained from touching fields behind conditional macros as
that quickly can get complicated.
@eriols eriols force-pushed the struct_packing_patch branch from 1ca43e9 to ca84251 Jan 19, 2021
@eriols
Copy link
Contributor Author

@eriols eriols commented Jan 19, 2021

@bagder I'll try to dig around to see if there are any automation possibilities within reach, but I figure it's a bit tricky to do. For projects that come in many flavors and for loads of architectures I figure this would mean running several parallel "packings" on different builds and then only applying the ones that do not mess up any other (important) configuration.
So that would probably be a manual call to make those trade-offs.
Also, running automated re-ordering would not support maintaining grouped variables together for readability as I tried to do here so that's another potential drawback.

@bagder
Copy link
Member

@bagder bagder commented Jan 19, 2021

A first step could be to just run it automatically and get some sort of daily summary or report or something, to see if we can learn from that and then see how we can proceed and improve. Probably on a chosen "default config" and platform or something.

@eriols
Copy link
Contributor Author

@eriols eriols commented Jan 19, 2021

Right, so just something ultra-crude like pahole --holes=1 lib/.libs/libcurl.a | perl -nle '$sum+=$1 if /XXX (\d+) bytes hole/; END {print $sum}' and if this goes above some threshold one might want to see if any modified or added structs should be reconsidered in terms of field arrangements.

@jay jay closed this in 0a58275 Jan 21, 2021
@jay
Copy link
Member

@jay jay commented Jan 21, 2021

Thanks

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants