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

classic Classic list List threaded Threaded
3 messages Options
Reply | Threaded
Open this post in threaded view
|

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

Samba - samba-technical mailing list
As suggested by Amitay, I mapped the code changes from ctdb_io
to ctdb's sock "API".

The attached patch series does contain a set of changes which might not
all be summarized under the above subject, however, the main driver of
those changes was the optimization of the memory handling/processing.

Even though this series is flagged as WIP, I believe they're not far
from being committable.

Review and feedback welcome.

Cheers Swen

ctdb_optimize_sock_memory_handling.patch (17K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

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

Samba - samba-technical mailing list
Hi Swen,

On Thu, 11 Jan 2018 13:06:00 +0100, Swen Schillig <[hidden email]>
wrote:

> As suggested by Amitay, I mapped the code changes from ctdb_io
> to ctdb's sock "API".
>
> The attached patch series does contain a set of changes which might not
> all be summarized under the above subject, however, the main driver of
> those changes was the optimization of the memory handling/processing.
>
> Even though this series is flagged as WIP, I believe they're not far
> from being committable.

As per our off-list discussion, I think commits 1-5 now look good.

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.  :-)

Before pushing them I would like to see some testing including some
performance numbers.  I would also like Amitay to have a look.

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.  :-)

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.

You almost certainly don't want "[PATCH 7/7] ctdb-common: Minor
cleanup.".  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.  You see this a lot in tevent_req-based code.  The more you see
it the more natural it becomes...  :-)

peace & happiness,
martin

Reply | Threaded
Open this post in threaded view
|

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

Samba - samba-technical mailing list
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

ctdb_optimize_sock_memory_handling_s2.patch (10K) Download Attachment