Change in osmo-msc[master]: large refactoring: support inter-BSC and inter-MSC Handover

Harald Welte gerrit-no-reply at lists.osmocom.org
Wed May 8 06:41:52 UTC 2019


Harald Welte has posted comments on this change. ( https://gerrit.osmocom.org/13137 )

Change subject: large refactoring: support inter-BSC and inter-MSC Handover
......................................................................


Patch Set 18: Code-Review+2

(1 comment)

You don't have to change the double-intialization right now before the merge, but I would ask you to create a ticket about it and look into replacing any such instances at some point in the future.

Same goes for Vadim's other comments [as far as they are valid].  Let's make sure you put something in place (ticket or whatever else) to not forget about those, thanks.

https://gerrit.osmocom.org/#/c/13137/9/src/libmsc/e_link.c
File src/libmsc/e_link.c:

https://gerrit.osmocom.org/#/c/13137/9/src/libmsc/e_link.c@77
PS9, Line 77: *e = (struct e_link) {
> I prefer this to clearly indicate that the struct is initialized from scratch. […]
the point is that the compiler is unable to optimize away initializing the same bit of memory *twice*, as talloc hapepns inside a library (and thus is outside of the scope of allocation).

I'm flexible to tolerate different preferences in syntax or code style to some extent, but when it comes with a runtime overhead, then I'm quite certain the variant causing the overhead is inferior to the veriant that doesn't.

So please kindly adjust your "common pattern" in a way that doesn't incur runtime overhead.  Either you don't use talloc_zero() but regular talloc(), or (very much preferred) don't initialize the entire struct all over again.

Sure, we have many other areas left for optimization, but there is absolutely zero advantage to initializing the memory twice to (mostly) zero.



-- 
To view, visit https://gerrit.osmocom.org/13137
To unsubscribe, or for help writing mail filters, visit https://gerrit.osmocom.org/settings

Gerrit-Project: osmo-msc
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I27e4988e0371808b512c757d2b52ada1615067bd
Gerrit-Change-Number: 13137
Gerrit-PatchSet: 18
Gerrit-Owner: Neels Hofmeyr <nhofmeyr at sysmocom.de>
Gerrit-Reviewer: Harald Welte <laforge at gnumonks.org>
Gerrit-Reviewer: Jenkins Builder (1000002)
Gerrit-Reviewer: Neels Hofmeyr <nhofmeyr at sysmocom.de>
Gerrit-Reviewer: Vadim Yanitskiy <axilirator at gmail.com>
Gerrit-CC: Pau Espin Pedrol <pespin at sysmocom.de>
Gerrit-Comment-Date: Wed, 08 May 2019 06:41:52 +0000
Gerrit-HasComments: Yes
Gerrit-HasLabels: Yes
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.osmocom.org/pipermail/gerrit-log/attachments/20190508/b65dc98f/attachment.html>


More information about the gerrit-log mailing list