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

hash: move key into hash struct to reduce mallocs #1376

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@bagder
Member

bagder commented Mar 31, 2017

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.

hash: move key into hash struct to reduce mallocs
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.
@mention-bot

This comment has been minimized.

Show comment
Hide comment
@mention-bot

mention-bot Mar 31, 2017

@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.

mention-bot commented Mar 31, 2017

@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 */

This comment has been minimized.

@jay

jay Apr 1, 2017

Member

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.

@jay

jay Apr 1, 2017

Member

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.

This comment has been minimized.

@bagder

bagder Apr 2, 2017

Member

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.

@bagder

bagder Apr 2, 2017

Member

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.

This comment has been minimized.

@jay

jay Apr 3, 2017

Member

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.

@jay

jay Apr 3, 2017

Member

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.

@bagder bagder closed this in 4f2e348 Apr 4, 2017

@bagder bagder deleted the bagder/hash-less-malloc branch Apr 4, 2017

@lock lock bot locked as resolved and limited conversation to collaborators May 23, 2018

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.