[WIP] ctdb-common: optimze sock's memory managament (was [RFC] [CTDB] Optimized memory handling)

[Swen Schillig]

Hi Martin
On Fri, 2018-01-12 at 07:17 +1100, Martin Schwenke wrote:
> As per our off-list discussion, I think commits 1-5 now look good.
Great...first step !
> 
> The only minor comment I have is that you probably want to flow the
> commit messages out to roughly 80 columns.  Some of the flow is
> weird.  I was leaving this superficial comment until last.  :-)
well the entered comments are even shorter (70 cols) but because it is
a patch-set git adds the "[PATCH 1/7] " to the subject which is
extending it to the ugly format.
Anyway, fixed.

> Before pushing them I would like to see some testing including some
> performance numbers.
Ok, do we have tests which can/should be used ?
>   I would also like Amitay to have a look.
Definitely.

> In "[PATCH 6/7] ctdb-common: Adding branch prediction" there is this:
> 
> -	if (! sock_queue_set_fd(queue, fd)) {
> +	if (unlikely(sock_queue_set_fd(queue, fd) == false)) {
> 
> I don't think you need to change it to a compare-with-false, which is
> rarely a good idea.  :-)
Hmmm, why is that ?
Comparing against "true" is a bad idea, I know, but why "false" ?
> 
> While touching that line it might be good to switch it to Samba's
> more
> verbose style, by assigning the function call result to a variable
> ("ok"
> or "status") and testing the variable in the if condition.  This can
> make it easier to step through code in a debugger.  The compiler can
> always optimise.
Ok, done.
> 
> You almost certainly don't want "[PATCH 7/7] ctdb-common: Minor
> cleanup.". 
I knew it, that's why tried to sneak that one in as last :-)
Removed.

>  Although the forward declarations look a bit weird, and
> possibly unnecessary, they are intentional.  The idea is that the
> flow of the code can be followed by reading downwards through the
> file. 
I was thinking having the externally available functions first and then
all static's is what is common...and then order them to try to prevent
forwards...but hey.
>  You see this a lot in tevent_req-based code.  The more you see
> it the more natural it becomes...  :-)
I'm afraid it will :-)

Patch series is updated.

Cheers Swen
-------------- next part --------------
A non-text attachment was scrubbed...
Name: ctdb_optimize_sock_memory_handling_s2.patch
Type: text/x-patch
Size: 7514 bytes
Desc: not available
URL: <http://lists.samba.org/pipermail/samba-technical/attachments/20180112/21058f1e/ctdb_optimize_sock_memory_handling_s2.bin>