hash: move key into hash struct to reduce mallocs #1376
Conversation
This removes one malloc for each hash struct allocated. In the simplest case like "curl localhost", my test case went from 112 mallocs to 109.
|
@bagder, thanks for your PR! By analyzing the history of the files in this pull request, we identified @yangtse, @dfandrich and @linusnielsen to be potential reviewers. |
| size_t key_len; | ||
| char key[1]; /* allocated memory following the struct */ |
There was a problem hiding this comment.
I think it would be safer to leave it as a char pointer:
diff --git a/lib/hash.c b/lib/hash.c
index 6fb0470..5f74749 100644
--- a/lib/hash.c
+++ b/lib/hash.c
@@ -100,7 +100,8 @@ mk_hash_element(const void *key, size_t key_len, const void *p)
struct curl_hash_element *he = malloc(sizeof(struct curl_hash_element) +
key_len);
if(he) {
- /* copy the key */
+ /* copy the key to the end of the struct */
+ he->key = (char *)he + sizeof(*he);
memcpy(he->key, key, key_len);
he->key_len = key_len;
he->ptr = (void *) p;
diff --git a/lib/hash.h b/lib/hash.h
index bc03672..169fba5 100644
--- a/lib/hash.h
+++ b/lib/hash.h
@@ -58,8 +58,8 @@ struct curl_hash {
struct curl_hash_element {
void *ptr;
+ char *key;
size_t key_len;
- char key[1]; /* allocated memory following the struct */
};
struct curl_hash_iterator {key[1] VLA isn't C89 and I think may cause bad behavior during compiler optimization since it's sized as 1. Either one of them may set off an analyzer.
Regarding this commit and the other one which seeks to borrow from the stack, did you profile, are these major offenders? I think sometimes these can do more harm than good, at the very least if they're harder to maintain.
There was a problem hiding this comment.
I'm set myself off on a little task to reduce the number of (unnecessary) mallocs, and I've started off with the really low hanging fruit like these ones. Really small and repeated allocations that we can skip. I don't think I've made the code much more complicated with this move, instead I actually think that the hash code gets slightly easier to read after this since we remove an error case.
I don't want to keep the pointer since without that, it is clearer that we don't need to free what it points to. The array-with-size-one trick is as old as the language C and has been used everywhere for decades, I don't consider that such a big deal.
There was a problem hiding this comment.
Oops I meant flexible array not variable length array VLA. We're going to have to agree to disagree.
The rule I'm thinking of is the one where it basically forbids accessing any array element outside of the range past end. In the case of *key declaration it doesn't know the size of the data but in the case of key[1] declaration it knows what it thinks is the size. One could argue key[1] is just going to decay key+n and char * access is universal; but I think since still it knows the range that's a problem.
I am 100% for making some exceptions to ignore fictional systems and make our lives easier, eg twos complement is actually everywhere, but I just don't know that this is one of those things. On MS compilers i know it's supported because some Windows API structs use it.
This removes one malloc for each hash struct allocated. In the simplest
case like "curl localhost", my test case went from 112 allocations to 109.