code review 110920043: syscall: NetlinkRIB - avoid allocation in loop

syscall: NetlinkRIB - avoid allocation in loop

NetlinkRIB is currently allocating a page sized slice of bytes in a
for loop and it's also calling Getpagesize() in the same for loop.

This CL changes NetlinkRIB to preallocate the page sized slice of
bytes before reaching the for loop. This reduces memory allocations
and lowers the number of calls to Getpagesize() to 1 per NetlinkRIB
call.

This CL reduces the allocated memory from 141.5 MB down to 52 MB in
a test.

[unclejack, 2014-07-08 13:49:17]

Hello dave@cheney.net, dsymonds@golang.org (cc: bradfitz@golang.org,
dsymonds@golang.org, golang-codereviews@googlegroups.com),

I'd like you to review this change to
https://code.google.com/p/go/

[crawshaw, 2014-07-08 14:56:20]

I don't see your name in golang.org/CONTRIBUTORS. Please sign the
individual CLA, as described in http://golang.org/doc/contribute.html

https://developers.google.com/open-source/cla/individual

On Tue, Jul 8, 2014 at 9:49 AM,  <unclejacksons@gmail.com> wrote:
> Reviewers: dfc, dsymonds,
>
> Message:
> Hello dave@cheney.net, dsymonds@golang.org (cc: bradfitz@golang.org,
> dsymonds@golang.org, golang-codereviews@googlegroups.com),
>
> I'd like you to review this change to
> https://code.google.com/p/go/
>
>
> Description:
> syscall: NetlinkRIB - avoid allocation in loop
>
> NetlinkRIB is currently allocating a page sized slice of bytes in a
> for loop and it's also calling Getpagesize() in the same for loop.
>
> This CL changes NetlinkRIB to preallocate the page sized slice of
> bytes before reaching the for loop. This reduces memory allocations
> and lowers the number of calls to Getpagesize() to 1 per NetlinkRIB
> call.
>
> This CL reduces the allocated memory from 141.5 MB down to 52 MB in a
> test.
>
> Please review this at https://codereview.appspot.com/110920043/
>
> Affected files (+2, -1 lines):
>   M src/pkg/syscall/netlink_linux.go
>
>
> Index: src/pkg/syscall/netlink_linux.go
> ===================================================================
> --- a/src/pkg/syscall/netlink_linux.go
> +++ b/src/pkg/syscall/netlink_linux.go
> @@ -64,9 +64,10 @@
>                 return nil, err
>         }
>         var tab []byte
> +       rbNew := make([]byte, Getpagesize())
>  done:
>         for {
> -               rb := make([]byte, Getpagesize())
> +               rb := rbNew
>                 nr, _, err := Recvfrom(s, rb, 0)
>                 if err != nil {
>                         return nil, err
>
>
> --
> You received this message because you are subscribed to the Google Groups
> "golang-codereviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to golang-codereviews+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.

[unclejack, 2014-07-08 15:20:10]

bradfitz has taken care of the CLA.

I've sent https://codereview.appspot.com/107440043 and
https://codereview.appspot.com/108240044 before. They've been committed.

On 2014/07/08 14:56:20, crawshaw wrote:
> I don't see your name in golang.org/CONTRIBUTORS. Please sign the
> individual CLA, as described in http://golang.org/doc/contribute.html
> 
> https://developers.google.com/open-source/cla/individual
> 
> On Tue, Jul 8, 2014 at 9:49 AM,  <mailto:unclejacksons@gmail.com> wrote:
> > Reviewers: dfc, dsymonds,
> >
> > Message:
> > Hello mailto:dave@cheney.net, mailto:dsymonds@golang.org (cc:
mailto:bradfitz@golang.org,
> > mailto:dsymonds@golang.org, mailto:golang-codereviews@googlegroups.com),
> >
> > I'd like you to review this change to
> > https://code.google.com/p/go/
> >
> >
> > Description:
> > syscall: NetlinkRIB - avoid allocation in loop
> >
> > NetlinkRIB is currently allocating a page sized slice of bytes in a
> > for loop and it's also calling Getpagesize() in the same for loop.
> >
> > This CL changes NetlinkRIB to preallocate the page sized slice of
> > bytes before reaching the for loop. This reduces memory allocations
> > and lowers the number of calls to Getpagesize() to 1 per NetlinkRIB
> > call.
> >
> > This CL reduces the allocated memory from 141.5 MB down to 52 MB in a
> > test.
> >
> > Please review this at https://codereview.appspot.com/110920043/
> >
> > Affected files (+2, -1 lines):
> >   M src/pkg/syscall/netlink_linux.go
> >
> >
> > Index: src/pkg/syscall/netlink_linux.go
> > ===================================================================
> > --- a/src/pkg/syscall/netlink_linux.go
> > +++ b/src/pkg/syscall/netlink_linux.go
> > @@ -64,9 +64,10 @@
> >                 return nil, err
> >         }
> >         var tab []byte
> > +       rbNew := make([]byte, Getpagesize())
> >  done:
> >         for {
> > -               rb := make([]byte, Getpagesize())
> > +               rb := rbNew
> >                 nr, _, err := Recvfrom(s, rb, 0)
> >                 if err != nil {
> >                         return nil, err
> >
> >
> > --
> > You received this message because you are subscribed to the Google Groups
> > "golang-codereviews" group.
> > To unsubscribe from this group and stop receiving emails from it, send an
> > email to mailto:golang-codereviews+unsubscribe@googlegroups.com.
> > For more options, visit https://groups.google.com/d/optout.

[crawshaw, 2014-07-08 15:22:26]

Apologies, I was looking in an old CONTRIBUTORS file.

On Tue, Jul 8, 2014 at 11:20 AM,  <unclejacksons@gmail.com> wrote:
> bradfitz has taken care of the CLA.
>
> I've sent https://codereview.appspot.com/107440043 and
> https://codereview.appspot.com/108240044 before. They've been committed.
>
>
> On 2014/07/08 14:56:20, crawshaw wrote:
>>
>> I don't see your name in golang.org/CONTRIBUTORS. Please sign the
>> individual CLA, as described in http://golang.org/doc/contribute.html
>
>
>> https://developers.google.com/open-source/cla/individual
>
>
>> On Tue, Jul 8, 2014 at 9:49 AM,  <mailto:unclejacksons@gmail.com>
>
> wrote:
>>
>> > Reviewers: dfc, dsymonds,
>> >
>> > Message:
>> > Hello mailto:dave@cheney.net, mailto:dsymonds@golang.org (cc:
>
> mailto:bradfitz@golang.org,
>>
>> > mailto:dsymonds@golang.org,
>
> mailto:golang-codereviews@googlegroups.com),
>
>> >
>> > I'd like you to review this change to
>> > https://code.google.com/p/go/
>> >
>> >
>> > Description:
>> > syscall: NetlinkRIB - avoid allocation in loop
>> >
>> > NetlinkRIB is currently allocating a page sized slice of bytes in a
>> > for loop and it's also calling Getpagesize() in the same for loop.
>> >
>> > This CL changes NetlinkRIB to preallocate the page sized slice of
>> > bytes before reaching the for loop. This reduces memory allocations
>> > and lowers the number of calls to Getpagesize() to 1 per NetlinkRIB
>> > call.
>> >
>> > This CL reduces the allocated memory from 141.5 MB down to 52 MB in
>
> a
>>
>> > test.
>> >
>> > Please review this at https://codereview.appspot.com/110920043/
>> >
>> > Affected files (+2, -1 lines):
>> >   M src/pkg/syscall/netlink_linux.go
>> >
>> >
>> > Index: src/pkg/syscall/netlink_linux.go
>> > ===================================================================
>> > --- a/src/pkg/syscall/netlink_linux.go
>> > +++ b/src/pkg/syscall/netlink_linux.go
>> > @@ -64,9 +64,10 @@
>> >                 return nil, err
>> >         }
>> >         var tab []byte
>> > +       rbNew := make([]byte, Getpagesize())
>> >  done:
>> >         for {
>> > -               rb := make([]byte, Getpagesize())
>> > +               rb := rbNew
>> >                 nr, _, err := Recvfrom(s, rb, 0)
>> >                 if err != nil {
>> >                         return nil, err
>> >
>> >
>> > --
>> > You received this message because you are subscribed to the Google
>
> Groups
>>
>> > "golang-codereviews" group.
>> > To unsubscribe from this group and stop receiving emails from it,
>
> send an
>>
>> > email to mailto:golang-codereviews+unsubscribe@googlegroups.com.
>>
>> > For more options, visit https://groups.google.com/d/optout.
>
>
>
>
> https://codereview.appspot.com/110920043/

[crawshaw, 2014-07-08 15:23:35]

LGTM

[dave_cheney.net, 2014-07-09 08:47:17]

LGTM. Submitting.

[dave_cheney.net, 2014-07-09 08:49:50]

On 2014/07/09 08:47:17, dfc wrote:
> LGTM. Submitting.

FWIW, Getpagesize always returns a constant and will be inlined into the caller
so effectively comes for free.

I think if you wanted to improve this function further you should try to avoid
the copying of rb into tab, possibly by sizing tab for a reasonably sized
result.

[dave_cheney.net, 2014-07-09 08:50:53]

*** Submitted as https://code.google.com/p/go/source/detail?r=fdd2d7a9dd4c ***

syscall: NetlinkRIB, avoid allocation in loop

NetlinkRIB is currently allocating a page sized slice of bytes in a
for loop and it's also calling Getpagesize() in the same for loop.

This CL changes NetlinkRIB to preallocate the page sized slice of
bytes before reaching the for loop. This reduces memory allocations
and lowers the number of calls to Getpagesize() to 1 per NetlinkRIB
call.

This CL reduces the allocated memory from 141.5 MB down to 52 MB in
a test.

LGTM=crawshaw, dave
R=dave, dsymonds, crawshaw
CC=bradfitz, dsymonds, golang-codereviews
https://codereview.appspot.com/110920043

Committer: Dave Cheney <dave@cheney.net>

[gobot, 2014-07-09 08:52:28]

This CL appears to have broken the linux-arm-cheney-panda builder.
See http://build.golang.org/log/64fd7831aa19942d31425a1552b7aa96e27c85e8

[dave_cheney.net, 2014-07-09 08:54:01]

# Building C bootstrap tool.
cmd/dist
./make.bash: line 132: clang: command not found
Build complete, duration 101.074219ms. Result: error: exit status 127

oops, sorry, this is an unrelated failure

On Wed, Jul 9, 2014 at 6:52 PM,  <gobot@golang.org> wrote:
> This CL appears to have broken the linux-arm-cheney-panda builder.
> See http://build.golang.org/log/64fd7831aa19942d31425a1552b7aa96e27c85e8
>
> https://codereview.appspot.com/110920043/