[LEDE-DEV] [PATCH] ubus: Remove unnecessary memset calls.
Alexandru Ardelean
ardeleanalex at gmail.com
Tue Nov 7 23:56:56 PST 2017
On Tue, Nov 7, 2017 at 11:18 PM, Arjen de Korte <arjen+lede at de-korte.org> wrote:
> Citeren Rosen Penev <rosenp at 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 at de-korte.org>
>> wrote:
>>>
>>> Citeren Rosen Penev <rosenp at 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 at 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 at lists.infradead.org
>>> http://lists.infradead.org/mailman/listinfo/lede-dev
>
>
>
>
>
> _______________________________________________
> Lede-dev mailing list
> Lede-dev at lists.infradead.org
> http://lists.infradead.org/mailman/listinfo/lede-dev
More information about the Lede-dev
mailing list