[LEDE-DEV] [PATCH] ubus: Remove unnecessary memset calls.
Arjen de Korte
arjen+lede at de-korte.org
Wed Nov 8 10:55:04 PST 2017
Citeren Alexandru Ardelean <ardeleanalex at gmail.com>:
> 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 :)
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 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
>
> _______________________________________________
> 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