| Message ID | 20171107203420.6439-1-rosenp@gmail.com |
|---|---|
| State | Accepted |
| Headers | show |
| Series | [LEDE-DEV] ubus: Remove unnecessary memset calls. | expand |
Citeren Rosen Penev <rosenp@gmail.com>: > Replace malloc+memset with calloc. Cleaner and faster in extreme situations. Calloc is definitly *not* faster than malloc + memset. Under the hood, calloc will call malloc, check if memory allocation was successful and then proceed to set all allocated memory to 0. You need to check the return value of malloc or calloc anyway, so this will be actually slower. It *may* be considered cleaner, since it will check if the memory allocation succeeded before writing zeros. > Signed-off-by: Rosen Penev <rosenp@gmail.com> > --- > libubus.c | 6 ++---- > lua/ubus.c | 18 ++++++------------ > 2 files changed, 8 insertions(+), 16 deletions(-) > > diff --git a/libubus.c b/libubus.c > index 9463522..260e40f 100644 > --- a/libubus.c > +++ b/libubus.c > @@ -133,7 +133,7 @@ struct ubus_lookup_request { > static void ubus_lookup_cb(struct ubus_request *ureq, int type, > struct blob_attr *msg) > { > struct ubus_lookup_request *req; > - struct ubus_object_data obj; > + struct ubus_object_data obj = {}; > struct blob_attr **attr; > > req = container_of(ureq, struct ubus_lookup_request, req); > @@ -143,7 +143,6 @@ static void ubus_lookup_cb(struct ubus_request > *ureq, int type, struct blob_attr > !attr[UBUS_ATTR_OBJTYPE]) > return; > > - memset(&obj, 0, sizeof(obj)); > obj.id = blob_get_u32(attr[UBUS_ATTR_OBJID]); > obj.path = blob_data(attr[UBUS_ATTR_OBJPATH]); > obj.type_id = blob_get_u32(attr[UBUS_ATTR_OBJTYPE]); > @@ -220,7 +219,7 @@ int ubus_register_event_handler(struct ubus_context *ctx, > const char *pattern) > { > struct ubus_object *obj = &ev->obj; > - struct blob_buf b2; > + struct blob_buf b2 = {}; > int ret; > > if (!obj->id) { > @@ -236,7 +235,6 @@ int ubus_register_event_handler(struct ubus_context *ctx, > } > > /* use a second buffer, ubus_invoke() overwrites the primary one */ > - memset(&b2, 0, sizeof(b2)); > blob_buf_init(&b2, 0); > blobmsg_add_u32(&b2, "object", obj->id); > if (pattern) > diff --git a/lua/ubus.c b/lua/ubus.c > index 74a15b0..e9fd10e 100644 > --- a/lua/ubus.c > +++ b/lua/ubus.c > @@ -410,11 +410,10 @@ static int ubus_lua_load_methods(lua_State *L, > struct ubus_method *m) > } > > /* setup the policy pointers */ > - p = malloc(sizeof(struct blobmsg_policy) * plen); > + p = calloc(plen, sizeof(struct blobmsg_policy)); > if (!p) > return 1; > > - memset(p, 0, sizeof(struct blobmsg_policy) * plen); > m->policy = p; > lua_pushnil(L); > while (lua_next(L, -2) != 0) { > @@ -481,26 +480,23 @@ static struct ubus_object* > ubus_lua_load_object(lua_State *L) > int midx = 0; > > /* setup object pointers */ > - obj = malloc(sizeof(struct ubus_lua_object)); > + obj = calloc(1, sizeof(struct ubus_lua_object)); > if (!obj) > return NULL; > > - memset(obj, 0, sizeof(struct ubus_lua_object)); > obj->o.name = lua_tostring(L, -2); > > /* setup method pointers */ > - m = malloc(sizeof(struct ubus_method) * mlen); > - memset(m, 0, sizeof(struct ubus_method) * mlen); > + m = calloc(1, sizeof(struct ubus_method) * mlen); > obj->o.methods = m; > > /* setup type pointers */ > - obj->o.type = malloc(sizeof(struct ubus_object_type)); > + obj->o.type = calloc(1, sizeof(struct ubus_object_type)); > if (!obj->o.type) { > free(obj); > return NULL; > } > > - memset(obj->o.type, 0, sizeof(struct ubus_object_type)); > obj->o.type->name = lua_tostring(L, -2); > obj->o.type->id = 0; > obj->o.type->methods = obj->o.methods; > @@ -715,11 +711,10 @@ ubus_lua_load_event(lua_State *L) > { > struct ubus_lua_event* event = NULL; > > - event = malloc(sizeof(struct ubus_lua_event)); > + event = calloc(1, sizeof(struct ubus_lua_event)); > if (!event) > return NULL; > > - memset(event, 0, sizeof(struct ubus_lua_event)); > event->e.cb = ubus_event_handler; > > /* update the he callback lookup table */ > @@ -818,8 +813,7 @@ ubus_lua_do_subscribe( struct ubus_context *ctx, > lua_State *L, const char* targe > lua_error( L ); > } > > - sub = malloc( sizeof( struct ubus_lua_subscriber ) ); > - memset( sub, 0, sizeof( struct ubus_lua_subscriber ) ); > + sub = calloc( 1, sizeof( struct ubus_lua_subscriber ) ); > if( !sub ){ > lua_pushstring( L, "Out of memory" ); > lua_error( L );
I beg to differ. https://vorpus.org/blog/why-does-calloc-exist/ Section 2. On Tue, Nov 7, 2017 at 12:46 PM, Arjen de Korte <arjen+lede@de-korte.org> wrote: > Citeren Rosen Penev <rosenp@gmail.com>: > >> Replace malloc+memset with calloc. Cleaner and faster in extreme >> situations. > > > Calloc is definitly *not* faster than malloc + memset. Under the hood, > calloc will call malloc, check if memory allocation was successful and then > proceed to set all allocated memory to 0. You need to check the return value > of malloc or calloc anyway, so this will be actually slower. > > It *may* be considered cleaner, since it will check if the memory allocation > succeeded before writing zeros. > > >> Signed-off-by: Rosen Penev <rosenp@gmail.com> >> --- >> libubus.c | 6 ++---- >> lua/ubus.c | 18 ++++++------------ >> 2 files changed, 8 insertions(+), 16 deletions(-) >> >> diff --git a/libubus.c b/libubus.c >> index 9463522..260e40f 100644 >> --- a/libubus.c >> +++ b/libubus.c >> @@ -133,7 +133,7 @@ struct ubus_lookup_request { >> static void ubus_lookup_cb(struct ubus_request *ureq, int type, struct >> blob_attr *msg) >> { >> struct ubus_lookup_request *req; >> - struct ubus_object_data obj; >> + struct ubus_object_data obj = {}; >> struct blob_attr **attr; >> >> req = container_of(ureq, struct ubus_lookup_request, req); >> @@ -143,7 +143,6 @@ static void ubus_lookup_cb(struct ubus_request *ureq, >> int type, struct blob_attr >> !attr[UBUS_ATTR_OBJTYPE]) >> return; >> >> - memset(&obj, 0, sizeof(obj)); >> obj.id = blob_get_u32(attr[UBUS_ATTR_OBJID]); >> obj.path = blob_data(attr[UBUS_ATTR_OBJPATH]); >> obj.type_id = blob_get_u32(attr[UBUS_ATTR_OBJTYPE]); >> @@ -220,7 +219,7 @@ int ubus_register_event_handler(struct ubus_context >> *ctx, >> const char *pattern) >> { >> struct ubus_object *obj = &ev->obj; >> - struct blob_buf b2; >> + struct blob_buf b2 = {}; >> int ret; >> >> if (!obj->id) { >> @@ -236,7 +235,6 @@ int ubus_register_event_handler(struct ubus_context >> *ctx, >> } >> >> /* use a second buffer, ubus_invoke() overwrites the primary one >> */ >> - memset(&b2, 0, sizeof(b2)); >> blob_buf_init(&b2, 0); >> blobmsg_add_u32(&b2, "object", obj->id); >> if (pattern) >> diff --git a/lua/ubus.c b/lua/ubus.c >> index 74a15b0..e9fd10e 100644 >> --- a/lua/ubus.c >> +++ b/lua/ubus.c >> @@ -410,11 +410,10 @@ static int ubus_lua_load_methods(lua_State *L, >> struct ubus_method *m) >> } >> >> /* setup the policy pointers */ >> - p = malloc(sizeof(struct blobmsg_policy) * plen); >> + p = calloc(plen, sizeof(struct blobmsg_policy)); >> if (!p) >> return 1; >> >> - memset(p, 0, sizeof(struct blobmsg_policy) * plen); >> m->policy = p; >> lua_pushnil(L); >> while (lua_next(L, -2) != 0) { >> @@ -481,26 +480,23 @@ static struct ubus_object* >> ubus_lua_load_object(lua_State *L) >> int midx = 0; >> >> /* setup object pointers */ >> - obj = malloc(sizeof(struct ubus_lua_object)); >> + obj = calloc(1, sizeof(struct ubus_lua_object)); >> if (!obj) >> return NULL; >> >> - memset(obj, 0, sizeof(struct ubus_lua_object)); >> obj->o.name = lua_tostring(L, -2); >> >> /* setup method pointers */ >> - m = malloc(sizeof(struct ubus_method) * mlen); >> - memset(m, 0, sizeof(struct ubus_method) * mlen); >> + m = calloc(1, sizeof(struct ubus_method) * mlen); >> obj->o.methods = m; >> >> /* setup type pointers */ >> - obj->o.type = malloc(sizeof(struct ubus_object_type)); >> + obj->o.type = calloc(1, sizeof(struct ubus_object_type)); >> if (!obj->o.type) { >> free(obj); >> return NULL; >> } >> >> - memset(obj->o.type, 0, sizeof(struct ubus_object_type)); >> obj->o.type->name = lua_tostring(L, -2); >> obj->o.type->id = 0; >> obj->o.type->methods = obj->o.methods; >> @@ -715,11 +711,10 @@ ubus_lua_load_event(lua_State *L) >> { >> struct ubus_lua_event* event = NULL; >> >> - event = malloc(sizeof(struct ubus_lua_event)); >> + event = calloc(1, sizeof(struct ubus_lua_event)); >> if (!event) >> return NULL; >> >> - memset(event, 0, sizeof(struct ubus_lua_event)); >> event->e.cb = ubus_event_handler; >> >> /* update the he callback lookup table */ >> @@ -818,8 +813,7 @@ ubus_lua_do_subscribe( struct ubus_context *ctx, >> lua_State *L, const char* targe >> lua_error( L ); >> } >> >> - sub = malloc( sizeof( struct ubus_lua_subscriber ) ); >> - memset( sub, 0, sizeof( struct ubus_lua_subscriber ) ); >> + sub = calloc( 1, sizeof( struct ubus_lua_subscriber ) ); >> if( !sub ){ >> lua_pushstring( L, "Out of memory" ); >> lua_error( L ); > > > > > > _______________________________________________ > Lede-dev mailing list > Lede-dev@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/lede-dev
Citeren Rosen Penev <rosenp@gmail.com>: > I beg to differ. https://vorpus.org/blog/why-does-calloc-exist/ > > Section 2. I don't care about theoretical gains, benchmarks please. How much do you gain with these patches? I really doubt that in any of these patches there will be a tangible gain. > On Tue, Nov 7, 2017 at 12:46 PM, Arjen de Korte > <arjen+lede@de-korte.org> wrote: >> Citeren Rosen Penev <rosenp@gmail.com>: >> >>> Replace malloc+memset with calloc. Cleaner and faster in extreme >>> situations. >> >> >> Calloc is definitly *not* faster than malloc + memset. Under the hood, >> calloc will call malloc, check if memory allocation was successful and then >> proceed to set all allocated memory to 0. You need to check the return value >> of malloc or calloc anyway, so this will be actually slower. >> >> It *may* be considered cleaner, since it will check if the memory allocation >> succeeded before writing zeros. >> >> >>> Signed-off-by: Rosen Penev <rosenp@gmail.com> >>> --- >>> libubus.c | 6 ++---- >>> lua/ubus.c | 18 ++++++------------ >>> 2 files changed, 8 insertions(+), 16 deletions(-) >>> >>> diff --git a/libubus.c b/libubus.c >>> index 9463522..260e40f 100644 >>> --- a/libubus.c >>> +++ b/libubus.c >>> @@ -133,7 +133,7 @@ struct ubus_lookup_request { >>> static void ubus_lookup_cb(struct ubus_request *ureq, int type, struct >>> blob_attr *msg) >>> { >>> struct ubus_lookup_request *req; >>> - struct ubus_object_data obj; >>> + struct ubus_object_data obj = {}; >>> struct blob_attr **attr; >>> >>> req = container_of(ureq, struct ubus_lookup_request, req); >>> @@ -143,7 +143,6 @@ static void ubus_lookup_cb(struct ubus_request *ureq, >>> int type, struct blob_attr >>> !attr[UBUS_ATTR_OBJTYPE]) >>> return; >>> >>> - memset(&obj, 0, sizeof(obj)); >>> obj.id = blob_get_u32(attr[UBUS_ATTR_OBJID]); >>> obj.path = blob_data(attr[UBUS_ATTR_OBJPATH]); >>> obj.type_id = blob_get_u32(attr[UBUS_ATTR_OBJTYPE]); >>> @@ -220,7 +219,7 @@ int ubus_register_event_handler(struct ubus_context >>> *ctx, >>> const char *pattern) >>> { >>> struct ubus_object *obj = &ev->obj; >>> - struct blob_buf b2; >>> + struct blob_buf b2 = {}; >>> int ret; >>> >>> if (!obj->id) { >>> @@ -236,7 +235,6 @@ int ubus_register_event_handler(struct ubus_context >>> *ctx, >>> } >>> >>> /* use a second buffer, ubus_invoke() overwrites the primary one >>> */ >>> - memset(&b2, 0, sizeof(b2)); >>> blob_buf_init(&b2, 0); >>> blobmsg_add_u32(&b2, "object", obj->id); >>> if (pattern) >>> diff --git a/lua/ubus.c b/lua/ubus.c >>> index 74a15b0..e9fd10e 100644 >>> --- a/lua/ubus.c >>> +++ b/lua/ubus.c >>> @@ -410,11 +410,10 @@ static int ubus_lua_load_methods(lua_State *L, >>> struct ubus_method *m) >>> } >>> >>> /* setup the policy pointers */ >>> - p = malloc(sizeof(struct blobmsg_policy) * plen); >>> + p = calloc(plen, sizeof(struct blobmsg_policy)); >>> if (!p) >>> return 1; >>> >>> - memset(p, 0, sizeof(struct blobmsg_policy) * plen); >>> m->policy = p; >>> lua_pushnil(L); >>> while (lua_next(L, -2) != 0) { >>> @@ -481,26 +480,23 @@ static struct ubus_object* >>> ubus_lua_load_object(lua_State *L) >>> int midx = 0; >>> >>> /* setup object pointers */ >>> - obj = malloc(sizeof(struct ubus_lua_object)); >>> + obj = calloc(1, sizeof(struct ubus_lua_object)); >>> if (!obj) >>> return NULL; >>> >>> - memset(obj, 0, sizeof(struct ubus_lua_object)); >>> obj->o.name = lua_tostring(L, -2); >>> >>> /* setup method pointers */ >>> - m = malloc(sizeof(struct ubus_method) * mlen); >>> - memset(m, 0, sizeof(struct ubus_method) * mlen); >>> + m = calloc(1, sizeof(struct ubus_method) * mlen); >>> obj->o.methods = m; >>> >>> /* setup type pointers */ >>> - obj->o.type = malloc(sizeof(struct ubus_object_type)); >>> + obj->o.type = calloc(1, sizeof(struct ubus_object_type)); >>> if (!obj->o.type) { >>> free(obj); >>> return NULL; >>> } >>> >>> - memset(obj->o.type, 0, sizeof(struct ubus_object_type)); >>> obj->o.type->name = lua_tostring(L, -2); >>> obj->o.type->id = 0; >>> obj->o.type->methods = obj->o.methods; >>> @@ -715,11 +711,10 @@ ubus_lua_load_event(lua_State *L) >>> { >>> struct ubus_lua_event* event = NULL; >>> >>> - event = malloc(sizeof(struct ubus_lua_event)); >>> + event = calloc(1, sizeof(struct ubus_lua_event)); >>> if (!event) >>> return NULL; >>> >>> - memset(event, 0, sizeof(struct ubus_lua_event)); >>> event->e.cb = ubus_event_handler; >>> >>> /* update the he callback lookup table */ >>> @@ -818,8 +813,7 @@ ubus_lua_do_subscribe( struct ubus_context *ctx, >>> lua_State *L, const char* targe >>> lua_error( L ); >>> } >>> >>> - sub = malloc( sizeof( struct ubus_lua_subscriber ) ); >>> - memset( sub, 0, sizeof( struct ubus_lua_subscriber ) ); >>> + sub = calloc( 1, sizeof( struct ubus_lua_subscriber ) ); >>> if( !sub ){ >>> lua_pushstring( L, "Out of memory" ); >>> lua_error( L ); >> >> >> >> >> >> _______________________________________________ >> Lede-dev mailing list >> Lede-dev@lists.infradead.org >> http://lists.infradead.org/mailman/listinfo/lede-dev
On Tue, Nov 7, 2017 at 11:18 PM, Arjen de Korte <arjen+lede@de-korte.org> wrote: > Citeren Rosen Penev <rosenp@gmail.com>: > >> I beg to differ. https://vorpus.org/blog/why-does-calloc-exist/ >> >> Section 2. > > > I don't care about theoretical gains, benchmarks please. How much do you > gain with these patches? I really doubt that in any of these patches there > will be a tangible gain. This feels like an opinion battle :) I'd also vote for calloc [vs malloc + memset] mostly for reduction of line numbers and/or cleaning up. I also feel that Rosen's argument for "faster in extreme situations" is somewhat "playing it by the ear". But I would not dismiss this based on that. These days compilers do so much optimization, it's hard to evaluate performance of one case vs another. For this case [specifically], we would need to see/check the assembly code for a few architectures [mips, powerpc, arm, x86, etc] to properly evaluate the performance improvement of calloc() vs malloc() + memset(). I feel that effort would be a too much for such a simple/trivial change. I would re-spin this with just the "cleaning up" part [since I feel there is more agreement on that]. But feel free do disagree with me :) > >> On Tue, Nov 7, 2017 at 12:46 PM, Arjen de Korte <arjen+lede@de-korte.org> >> wrote: >>> >>> Citeren Rosen Penev <rosenp@gmail.com>: >>> >>>> Replace malloc+memset with calloc. Cleaner and faster in extreme >>>> situations. >>> >>> >>> >>> Calloc is definitly *not* faster than malloc + memset. Under the hood, >>> calloc will call malloc, check if memory allocation was successful and >>> then >>> proceed to set all allocated memory to 0. You need to check the return >>> value >>> of malloc or calloc anyway, so this will be actually slower. >>> >>> It *may* be considered cleaner, since it will check if the memory >>> allocation >>> succeeded before writing zeros. >>> >>> >>>> Signed-off-by: Rosen Penev <rosenp@gmail.com> >>>> --- >>>> libubus.c | 6 ++---- >>>> lua/ubus.c | 18 ++++++------------ >>>> 2 files changed, 8 insertions(+), 16 deletions(-) >>>> >>>> diff --git a/libubus.c b/libubus.c >>>> index 9463522..260e40f 100644 >>>> --- a/libubus.c >>>> +++ b/libubus.c >>>> @@ -133,7 +133,7 @@ struct ubus_lookup_request { >>>> static void ubus_lookup_cb(struct ubus_request *ureq, int type, struct >>>> blob_attr *msg) >>>> { >>>> struct ubus_lookup_request *req; >>>> - struct ubus_object_data obj; >>>> + struct ubus_object_data obj = {}; >>>> struct blob_attr **attr; >>>> >>>> req = container_of(ureq, struct ubus_lookup_request, req); >>>> @@ -143,7 +143,6 @@ static void ubus_lookup_cb(struct ubus_request >>>> *ureq, >>>> int type, struct blob_attr >>>> !attr[UBUS_ATTR_OBJTYPE]) >>>> return; >>>> >>>> - memset(&obj, 0, sizeof(obj)); >>>> obj.id = blob_get_u32(attr[UBUS_ATTR_OBJID]); >>>> obj.path = blob_data(attr[UBUS_ATTR_OBJPATH]); >>>> obj.type_id = blob_get_u32(attr[UBUS_ATTR_OBJTYPE]); >>>> @@ -220,7 +219,7 @@ int ubus_register_event_handler(struct ubus_context >>>> *ctx, >>>> const char *pattern) >>>> { >>>> struct ubus_object *obj = &ev->obj; >>>> - struct blob_buf b2; >>>> + struct blob_buf b2 = {}; >>>> int ret; >>>> >>>> if (!obj->id) { >>>> @@ -236,7 +235,6 @@ int ubus_register_event_handler(struct ubus_context >>>> *ctx, >>>> } >>>> >>>> /* use a second buffer, ubus_invoke() overwrites the primary one >>>> */ >>>> - memset(&b2, 0, sizeof(b2)); >>>> blob_buf_init(&b2, 0); >>>> blobmsg_add_u32(&b2, "object", obj->id); >>>> if (pattern) >>>> diff --git a/lua/ubus.c b/lua/ubus.c >>>> index 74a15b0..e9fd10e 100644 >>>> --- a/lua/ubus.c >>>> +++ b/lua/ubus.c >>>> @@ -410,11 +410,10 @@ static int ubus_lua_load_methods(lua_State *L, >>>> struct ubus_method *m) >>>> } >>>> >>>> /* setup the policy pointers */ >>>> - p = malloc(sizeof(struct blobmsg_policy) * plen); >>>> + p = calloc(plen, sizeof(struct blobmsg_policy)); >>>> if (!p) >>>> return 1; >>>> >>>> - memset(p, 0, sizeof(struct blobmsg_policy) * plen); >>>> m->policy = p; >>>> lua_pushnil(L); >>>> while (lua_next(L, -2) != 0) { >>>> @@ -481,26 +480,23 @@ static struct ubus_object* >>>> ubus_lua_load_object(lua_State *L) >>>> int midx = 0; >>>> >>>> /* setup object pointers */ >>>> - obj = malloc(sizeof(struct ubus_lua_object)); >>>> + obj = calloc(1, sizeof(struct ubus_lua_object)); >>>> if (!obj) >>>> return NULL; >>>> >>>> - memset(obj, 0, sizeof(struct ubus_lua_object)); >>>> obj->o.name = lua_tostring(L, -2); >>>> >>>> /* setup method pointers */ >>>> - m = malloc(sizeof(struct ubus_method) * mlen); >>>> - memset(m, 0, sizeof(struct ubus_method) * mlen); >>>> + m = calloc(1, sizeof(struct ubus_method) * mlen); >>>> obj->o.methods = m; >>>> >>>> /* setup type pointers */ >>>> - obj->o.type = malloc(sizeof(struct ubus_object_type)); >>>> + obj->o.type = calloc(1, sizeof(struct ubus_object_type)); >>>> if (!obj->o.type) { >>>> free(obj); >>>> return NULL; >>>> } >>>> >>>> - memset(obj->o.type, 0, sizeof(struct ubus_object_type)); >>>> obj->o.type->name = lua_tostring(L, -2); >>>> obj->o.type->id = 0; >>>> obj->o.type->methods = obj->o.methods; >>>> @@ -715,11 +711,10 @@ ubus_lua_load_event(lua_State *L) >>>> { >>>> struct ubus_lua_event* event = NULL; >>>> >>>> - event = malloc(sizeof(struct ubus_lua_event)); >>>> + event = calloc(1, sizeof(struct ubus_lua_event)); >>>> if (!event) >>>> return NULL; >>>> >>>> - memset(event, 0, sizeof(struct ubus_lua_event)); >>>> event->e.cb = ubus_event_handler; >>>> >>>> /* update the he callback lookup table */ >>>> @@ -818,8 +813,7 @@ ubus_lua_do_subscribe( struct ubus_context *ctx, >>>> lua_State *L, const char* targe >>>> lua_error( L ); >>>> } >>>> >>>> - sub = malloc( sizeof( struct ubus_lua_subscriber ) ); >>>> - memset( sub, 0, sizeof( struct ubus_lua_subscriber ) ); >>>> + sub = calloc( 1, sizeof( struct ubus_lua_subscriber ) ); >>>> if( !sub ){ >>>> lua_pushstring( L, "Out of memory" ); >>>> lua_error( L ); >>> >>> >>> >>> >>> >>> >>> _______________________________________________ >>> Lede-dev mailing list >>> Lede-dev@lists.infradead.org >>> http://lists.infradead.org/mailman/listinfo/lede-dev > > > > > > _______________________________________________ > Lede-dev mailing list > Lede-dev@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/lede-dev
On Tue, Nov 7, 2017 at 9:34 PM, Rosen Penev <rosenp@gmail.com> wrote: > Replace malloc+memset with calloc. Cleaner and faster in extreme situations. > > Signed-off-by: Rosen Penev <rosenp@gmail.com> > --- > libubus.c | 6 ++---- > lua/ubus.c | 18 ++++++------------ > 2 files changed, 8 insertions(+), 16 deletions(-) > > diff --git a/libubus.c b/libubus.c > index 9463522..260e40f 100644 > --- a/libubus.c > +++ b/libubus.c > @@ -133,7 +133,7 @@ struct ubus_lookup_request { > static void ubus_lookup_cb(struct ubus_request *ureq, int type, struct blob_attr *msg) > { > struct ubus_lookup_request *req; > - struct ubus_object_data obj; > + struct ubus_object_data obj = {}; > struct blob_attr **attr; > > req = container_of(ureq, struct ubus_lookup_request, req); > @@ -143,7 +143,6 @@ static void ubus_lookup_cb(struct ubus_request *ureq, int type, struct blob_attr > !attr[UBUS_ATTR_OBJTYPE]) > return; > > - memset(&obj, 0, sizeof(obj)); > obj.id = blob_get_u32(attr[UBUS_ATTR_OBJID]); > obj.path = blob_data(attr[UBUS_ATTR_OBJPATH]); > obj.type_id = blob_get_u32(attr[UBUS_ATTR_OBJTYPE]); > @@ -220,7 +219,7 @@ int ubus_register_event_handler(struct ubus_context *ctx, > const char *pattern) > { > struct ubus_object *obj = &ev->obj; > - struct blob_buf b2; > + struct blob_buf b2 = {}; > int ret; > > if (!obj->id) { > @@ -236,7 +235,6 @@ int ubus_register_event_handler(struct ubus_context *ctx, > } > > /* use a second buffer, ubus_invoke() overwrites the primary one */ > - memset(&b2, 0, sizeof(b2)); > blob_buf_init(&b2, 0); > blobmsg_add_u32(&b2, "object", obj->id); > if (pattern) > diff --git a/lua/ubus.c b/lua/ubus.c > index 74a15b0..e9fd10e 100644 > --- a/lua/ubus.c > +++ b/lua/ubus.c > @@ -410,11 +410,10 @@ static int ubus_lua_load_methods(lua_State *L, struct ubus_method *m) > } > > /* setup the policy pointers */ > - p = malloc(sizeof(struct blobmsg_policy) * plen); > + p = calloc(plen, sizeof(struct blobmsg_policy)); > if (!p) > return 1; > > - memset(p, 0, sizeof(struct blobmsg_policy) * plen); > m->policy = p; > lua_pushnil(L); > while (lua_next(L, -2) != 0) { > @@ -481,26 +480,23 @@ static struct ubus_object* ubus_lua_load_object(lua_State *L) > int midx = 0; > > /* setup object pointers */ > - obj = malloc(sizeof(struct ubus_lua_object)); > + obj = calloc(1, sizeof(struct ubus_lua_object)); > if (!obj) > return NULL; > > - memset(obj, 0, sizeof(struct ubus_lua_object)); > obj->o.name = lua_tostring(L, -2); > > /* setup method pointers */ > - m = malloc(sizeof(struct ubus_method) * mlen); > - memset(m, 0, sizeof(struct ubus_method) * mlen); > + m = calloc(1, sizeof(struct ubus_method) * mlen); > obj->o.methods = m; While I have no strong opinion on the malloc + memset vs. calloc debate, here it looks like a check for if (!m) is missing. -Kristian
Citeren Alexandru Ardelean <ardeleanalex@gmail.com>: > On Tue, Nov 7, 2017 at 11:18 PM, Arjen de Korte > <arjen+lede@de-korte.org> wrote: >> Citeren Rosen Penev <rosenp@gmail.com>: >> >>> I beg to differ. https://vorpus.org/blog/why-does-calloc-exist/ >>> >>> Section 2. >> >> >> I don't care about theoretical gains, benchmarks please. How much do you >> gain with these patches? I really doubt that in any of these patches there >> will be a tangible gain. > > This feels like an opinion battle :) > > I'd also vote for calloc [vs malloc + memset] mostly for reduction of > line numbers and/or cleaning up. > > I also feel that Rosen's argument for "faster in extreme situations" > is somewhat "playing it by the ear". > But I would not dismiss this based on that. > > These days compilers do so much optimization, it's hard to evaluate > performance of one case vs another. > For this case [specifically], we would need to see/check the assembly > code for a few architectures [mips, powerpc, arm, x86, etc] to > properly evaluate the performance improvement of calloc() vs malloc() > + memset(). > I feel that effort would be a too much for such a simple/trivial change. > > I would re-spin this with just the "cleaning up" part [since I feel > there is more agreement on that]. > > But feel free do disagree with me :) I really see no point in implementing these patches. I've seen the backlash of far too many efficiency improvements, that either didn't live up to the expectations or outright introduced new bugs, that for anyone claiming something to be better, I want to see proof that it actually improves something *and* does not break something else. The commit messages I have seen so far provide neither. "If it ain't broke, don't fix it". >> >>> On Tue, Nov 7, 2017 at 12:46 PM, Arjen de Korte <arjen+lede@de-korte.org> >>> wrote: >>>> >>>> Citeren Rosen Penev <rosenp@gmail.com>: >>>> >>>>> Replace malloc+memset with calloc. Cleaner and faster in extreme >>>>> situations. >>>> >>>> >>>> >>>> Calloc is definitly *not* faster than malloc + memset. Under the hood, >>>> calloc will call malloc, check if memory allocation was successful and >>>> then >>>> proceed to set all allocated memory to 0. You need to check the return >>>> value >>>> of malloc or calloc anyway, so this will be actually slower. >>>> >>>> It *may* be considered cleaner, since it will check if the memory >>>> allocation >>>> succeeded before writing zeros. >>>> >>>> >>>>> Signed-off-by: Rosen Penev <rosenp@gmail.com> >>>>> --- >>>>> libubus.c | 6 ++---- >>>>> lua/ubus.c | 18 ++++++------------ >>>>> 2 files changed, 8 insertions(+), 16 deletions(-) >>>>> >>>>> diff --git a/libubus.c b/libubus.c >>>>> index 9463522..260e40f 100644 >>>>> --- a/libubus.c >>>>> +++ b/libubus.c >>>>> @@ -133,7 +133,7 @@ struct ubus_lookup_request { >>>>> static void ubus_lookup_cb(struct ubus_request *ureq, int type, struct >>>>> blob_attr *msg) >>>>> { >>>>> struct ubus_lookup_request *req; >>>>> - struct ubus_object_data obj; >>>>> + struct ubus_object_data obj = {}; >>>>> struct blob_attr **attr; >>>>> >>>>> req = container_of(ureq, struct ubus_lookup_request, req); >>>>> @@ -143,7 +143,6 @@ static void ubus_lookup_cb(struct ubus_request >>>>> *ureq, >>>>> int type, struct blob_attr >>>>> !attr[UBUS_ATTR_OBJTYPE]) >>>>> return; >>>>> >>>>> - memset(&obj, 0, sizeof(obj)); >>>>> obj.id = blob_get_u32(attr[UBUS_ATTR_OBJID]); >>>>> obj.path = blob_data(attr[UBUS_ATTR_OBJPATH]); >>>>> obj.type_id = blob_get_u32(attr[UBUS_ATTR_OBJTYPE]); >>>>> @@ -220,7 +219,7 @@ int ubus_register_event_handler(struct ubus_context >>>>> *ctx, >>>>> const char *pattern) >>>>> { >>>>> struct ubus_object *obj = &ev->obj; >>>>> - struct blob_buf b2; >>>>> + struct blob_buf b2 = {}; >>>>> int ret; >>>>> >>>>> if (!obj->id) { >>>>> @@ -236,7 +235,6 @@ int ubus_register_event_handler(struct ubus_context >>>>> *ctx, >>>>> } >>>>> >>>>> /* use a second buffer, ubus_invoke() overwrites the primary one >>>>> */ >>>>> - memset(&b2, 0, sizeof(b2)); >>>>> blob_buf_init(&b2, 0); >>>>> blobmsg_add_u32(&b2, "object", obj->id); >>>>> if (pattern) >>>>> diff --git a/lua/ubus.c b/lua/ubus.c >>>>> index 74a15b0..e9fd10e 100644 >>>>> --- a/lua/ubus.c >>>>> +++ b/lua/ubus.c >>>>> @@ -410,11 +410,10 @@ static int ubus_lua_load_methods(lua_State *L, >>>>> struct ubus_method *m) >>>>> } >>>>> >>>>> /* setup the policy pointers */ >>>>> - p = malloc(sizeof(struct blobmsg_policy) * plen); >>>>> + p = calloc(plen, sizeof(struct blobmsg_policy)); >>>>> if (!p) >>>>> return 1; >>>>> >>>>> - memset(p, 0, sizeof(struct blobmsg_policy) * plen); >>>>> m->policy = p; >>>>> lua_pushnil(L); >>>>> while (lua_next(L, -2) != 0) { >>>>> @@ -481,26 +480,23 @@ static struct ubus_object* >>>>> ubus_lua_load_object(lua_State *L) >>>>> int midx = 0; >>>>> >>>>> /* setup object pointers */ >>>>> - obj = malloc(sizeof(struct ubus_lua_object)); >>>>> + obj = calloc(1, sizeof(struct ubus_lua_object)); >>>>> if (!obj) >>>>> return NULL; >>>>> >>>>> - memset(obj, 0, sizeof(struct ubus_lua_object)); >>>>> obj->o.name = lua_tostring(L, -2); >>>>> >>>>> /* setup method pointers */ >>>>> - m = malloc(sizeof(struct ubus_method) * mlen); >>>>> - memset(m, 0, sizeof(struct ubus_method) * mlen); >>>>> + m = calloc(1, sizeof(struct ubus_method) * mlen); >>>>> obj->o.methods = m; >>>>> >>>>> /* setup type pointers */ >>>>> - obj->o.type = malloc(sizeof(struct ubus_object_type)); >>>>> + obj->o.type = calloc(1, sizeof(struct ubus_object_type)); >>>>> if (!obj->o.type) { >>>>> free(obj); >>>>> return NULL; >>>>> } >>>>> >>>>> - memset(obj->o.type, 0, sizeof(struct ubus_object_type)); >>>>> obj->o.type->name = lua_tostring(L, -2); >>>>> obj->o.type->id = 0; >>>>> obj->o.type->methods = obj->o.methods; >>>>> @@ -715,11 +711,10 @@ ubus_lua_load_event(lua_State *L) >>>>> { >>>>> struct ubus_lua_event* event = NULL; >>>>> >>>>> - event = malloc(sizeof(struct ubus_lua_event)); >>>>> + event = calloc(1, sizeof(struct ubus_lua_event)); >>>>> if (!event) >>>>> return NULL; >>>>> >>>>> - memset(event, 0, sizeof(struct ubus_lua_event)); >>>>> event->e.cb = ubus_event_handler; >>>>> >>>>> /* update the he callback lookup table */ >>>>> @@ -818,8 +813,7 @@ ubus_lua_do_subscribe( struct ubus_context *ctx, >>>>> lua_State *L, const char* targe >>>>> lua_error( L ); >>>>> } >>>>> >>>>> - sub = malloc( sizeof( struct ubus_lua_subscriber ) ); >>>>> - memset( sub, 0, sizeof( struct ubus_lua_subscriber ) ); >>>>> + sub = calloc( 1, sizeof( struct ubus_lua_subscriber ) ); >>>>> if( !sub ){ >>>>> lua_pushstring( L, "Out of memory" ); >>>>> lua_error( L ); >>>> >>>> >>>> >>>> >>>> >>>> >>>> _______________________________________________ >>>> Lede-dev mailing list >>>> Lede-dev@lists.infradead.org >>>> http://lists.infradead.org/mailman/listinfo/lede-dev >> >> >> >> >> >> _______________________________________________ >> Lede-dev mailing list >> Lede-dev@lists.infradead.org >> http://lists.infradead.org/mailman/listinfo/lede-dev > > _______________________________________________ > Lede-dev mailing list > Lede-dev@lists.infradead.org > http://lists.infradead.org/mailman/listinfo/lede-dev
diff --git a/libubus.c b/libubus.c index 9463522..260e40f 100644 --- a/libubus.c +++ b/libubus.c @@ -133,7 +133,7 @@ struct ubus_lookup_request { static void ubus_lookup_cb(struct ubus_request *ureq, int type, struct blob_attr *msg) { struct ubus_lookup_request *req; - struct ubus_object_data obj; + struct ubus_object_data obj = {}; struct blob_attr **attr; req = container_of(ureq, struct ubus_lookup_request, req); @@ -143,7 +143,6 @@ static void ubus_lookup_cb(struct ubus_request *ureq, int type, struct blob_attr !attr[UBUS_ATTR_OBJTYPE]) return; - memset(&obj, 0, sizeof(obj)); obj.id = blob_get_u32(attr[UBUS_ATTR_OBJID]); obj.path = blob_data(attr[UBUS_ATTR_OBJPATH]); obj.type_id = blob_get_u32(attr[UBUS_ATTR_OBJTYPE]); @@ -220,7 +219,7 @@ int ubus_register_event_handler(struct ubus_context *ctx, const char *pattern) { struct ubus_object *obj = &ev->obj; - struct blob_buf b2; + struct blob_buf b2 = {}; int ret; if (!obj->id) { @@ -236,7 +235,6 @@ int ubus_register_event_handler(struct ubus_context *ctx, } /* use a second buffer, ubus_invoke() overwrites the primary one */ - memset(&b2, 0, sizeof(b2)); blob_buf_init(&b2, 0); blobmsg_add_u32(&b2, "object", obj->id); if (pattern) diff --git a/lua/ubus.c b/lua/ubus.c index 74a15b0..e9fd10e 100644 --- a/lua/ubus.c +++ b/lua/ubus.c @@ -410,11 +410,10 @@ static int ubus_lua_load_methods(lua_State *L, struct ubus_method *m) } /* setup the policy pointers */ - p = malloc(sizeof(struct blobmsg_policy) * plen); + p = calloc(plen, sizeof(struct blobmsg_policy)); if (!p) return 1; - memset(p, 0, sizeof(struct blobmsg_policy) * plen); m->policy = p; lua_pushnil(L); while (lua_next(L, -2) != 0) { @@ -481,26 +480,23 @@ static struct ubus_object* ubus_lua_load_object(lua_State *L) int midx = 0; /* setup object pointers */ - obj = malloc(sizeof(struct ubus_lua_object)); + obj = calloc(1, sizeof(struct ubus_lua_object)); if (!obj) return NULL; - memset(obj, 0, sizeof(struct ubus_lua_object)); obj->o.name = lua_tostring(L, -2); /* setup method pointers */ - m = malloc(sizeof(struct ubus_method) * mlen); - memset(m, 0, sizeof(struct ubus_method) * mlen); + m = calloc(1, sizeof(struct ubus_method) * mlen); obj->o.methods = m; /* setup type pointers */ - obj->o.type = malloc(sizeof(struct ubus_object_type)); + obj->o.type = calloc(1, sizeof(struct ubus_object_type)); if (!obj->o.type) { free(obj); return NULL; } - memset(obj->o.type, 0, sizeof(struct ubus_object_type)); obj->o.type->name = lua_tostring(L, -2); obj->o.type->id = 0; obj->o.type->methods = obj->o.methods; @@ -715,11 +711,10 @@ ubus_lua_load_event(lua_State *L) { struct ubus_lua_event* event = NULL; - event = malloc(sizeof(struct ubus_lua_event)); + event = calloc(1, sizeof(struct ubus_lua_event)); if (!event) return NULL; - memset(event, 0, sizeof(struct ubus_lua_event)); event->e.cb = ubus_event_handler; /* update the he callback lookup table */ @@ -818,8 +813,7 @@ ubus_lua_do_subscribe( struct ubus_context *ctx, lua_State *L, const char* targe lua_error( L ); } - sub = malloc( sizeof( struct ubus_lua_subscriber ) ); - memset( sub, 0, sizeof( struct ubus_lua_subscriber ) ); + sub = calloc( 1, sizeof( struct ubus_lua_subscriber ) ); if( !sub ){ lua_pushstring( L, "Out of memory" ); lua_error( L );
Replace malloc+memset with calloc. Cleaner and faster in extreme situations. Signed-off-by: Rosen Penev <rosenp@gmail.com> --- libubus.c | 6 ++---- lua/ubus.c | 18 ++++++------------ 2 files changed, 8 insertions(+), 16 deletions(-)