Skip to content

Reduce allocs in ReadMessage (unix transport)#347

Merged
guelfey merged 6 commits intogodbus:masterfrom
marselester:reduce-unix-read-allocs
Dec 17, 2022
Merged

Reduce allocs in ReadMessage (unix transport)#347
guelfey merged 6 commits intogodbus:masterfrom
marselester:reduce-unix-read-allocs

Conversation

@marselester
Copy link
Contributor

@marselester marselester commented Dec 5, 2022 •
edited
Loading

It appeared to me that certain decoding performed in ReadMessage was repeated in DecodeMessageWithFDs, so I got rid of it. That yielded 58 less allocs, though consumed 5.63KB more per operation according to the benchmark.

# new
BenchmarkUnixFDs-2   	    3668	    395654 ns/op	   30512 B/op	     849 allocs/op
# old
BenchmarkUnixFDs-2   	    6574	    330534 ns/op	   24737 B/op	     907 allocs/op

It seems more memory is spent at syscall.anyToSockaddr after refactoring, see the profiles below.

new

Details 
Showing nodes accounting for 208.52MB, 70.08% of 297.53MB total
Dropped 16 nodes (cum <= 1.49MB)
Showing top 20 nodes out of 74
      flat  flat%   sum%        cum   cum%
    0.50MB  0.17%  0.17%   234.02MB 78.66%  github.com/godbus/dbus/v5.(*Conn).inWorker
       7MB  2.35%  2.52%   233.52MB 78.49%  github.com/godbus/dbus/v5.(*unixTransport).ReadMessage
    4.50MB  1.51%  4.03%   164.01MB 55.13%  github.com/godbus/dbus/v5.(*decoder).Decode
      20MB  6.72% 10.76%   159.01MB 53.44%  github.com/godbus/dbus/v5.(*decoder).decode
         0     0% 10.76%   135.01MB 45.38%  github.com/godbus/dbus/v5.(*oobReader).Read
         0     0% 10.76%   135.01MB 45.38%  net.(*UnixConn).ReadMsgUnix
      27MB  9.08% 19.83%   135.01MB 45.38%  net.(*UnixConn).readMsg
         0     0% 19.83%   111.51MB 37.48%  io.(*multiReader).Read
         0     0% 19.83%   108.01MB 36.30%  internal/poll.(*FD).ReadMsg
         0     0% 19.83%   108.01MB 36.30%  net.(*netFD).readMsg
         0     0% 19.83%   108.01MB 36.30%  syscall.Recvmsg
  108.01MB 36.30% 56.13%   108.01MB 36.30%  syscall.anyToSockaddr
      20MB  6.72% 62.86%    78.01MB 26.22%  github.com/godbus/dbus/v5.(*Message).EncodeToWithFDs
         0     0% 62.86%    68.51MB 23.03%  io.ReadAtLeast
         0     0% 62.86%    68.51MB 23.03%  io.ReadFull (inline)
         0     0% 62.86%       56MB 18.82%  github.com/godbus/dbus/v5.(*Message).EncodeTo (inline)
    2.50MB  0.84% 63.70%       49MB 16.47%  github.com/godbus/dbus/v5.(*Message).IsValid (inline)
         0     0% 63.70%       48MB 16.13%  github.com/godbus/dbus/v5.(*encoder).Encode
   17.50MB  5.88% 69.58%       48MB 16.13%  github.com/godbus/dbus/v5.(*encoder).encode
    1.50MB   0.5% 70.08%       46MB 15.46%  github.com/godbus/dbus/v5.(*decoder).read2buf

old

Details 
Showing nodes accounting for 72.01MB, 42.73% of 168.51MB total
Dropped 11 nodes (cum <= 0.84MB)
Showing top 20 nodes out of 75
      flat  flat%   sum%        cum   cum%
         0     0%     0%   120.01MB 71.22%  github.com/godbus/dbus/v5.(*Conn).inWorker
       3MB  1.78%  1.78%   120.01MB 71.22%  github.com/godbus/dbus/v5.(*unixTransport).ReadMessage
    4.50MB  2.67%  4.45%    75.01MB 44.51%  github.com/godbus/dbus/v5.DecodeMessageWithFDs
    5.50MB  3.26%  7.72%       55MB 32.64%  github.com/godbus/dbus/v5.(*decoder).Decode
      11MB  6.53% 14.24%       52MB 30.86%  github.com/godbus/dbus/v5.(*Message).EncodeToWithFDs
   23.50MB 13.95% 28.19%    48.50MB 28.78%  github.com/godbus/dbus/v5.(*decoder).decode
         0     0% 28.19%    43.50MB 25.82%  github.com/godbus/dbus/v5.(*Message).EncodeTo (inline)
       1MB  0.59% 28.78%    34.50MB 20.47%  github.com/godbus/dbus/v5.(*Message).IsValid (inline)
         0     0% 28.78%    34.50MB 20.47%  github.com/godbus/dbus/v5.(*encoder).Encode
      10MB  5.93% 34.72%    34.50MB 20.47%  github.com/godbus/dbus/v5.(*encoder).encode
         0     0% 34.72%       31MB 18.40%  github.com/godbus/dbus/v5.DecodeMessage (inline)
    5.50MB  3.26% 37.98%       30MB 17.80%  github.com/godbus/dbus/v5.(*Conn).handleCall
         0     0% 37.98%       20MB 11.87%  github.com/godbus/dbus/v5.(*Conn).sendMessageAndIfClosed
         0     0% 37.98%       20MB 11.87%  github.com/godbus/dbus/v5.(*outputHandler).sendAndIfClosed
    0.50MB   0.3% 38.28%       20MB 11.87%  github.com/godbus/dbus/v5.(*unixTransport).SendMessage
         0     0% 38.28%       19MB 11.28%  bytes.(*Buffer).Write
       6MB  3.56% 41.84%       19MB 11.28%  bytes.(*Buffer).grow
    1.50MB  0.89% 42.73%    17.50MB 10.39%  github.com/godbus/dbus/v5.(*Object).Call
         0     0% 42.73%    17.50MB 10.39%  github.com/godbus/dbus/v5.BenchmarkUnixFDs
         0     0% 42.73%    17.50MB 10.39%  testing.(*B).launch

@marselester marselester mentioned this pull request Dec 5, 2022
Closed
@marselester marselester force-pushed the reduce-unix-read-allocs branch from 9625d0b to 610d8b4 Compare December 6, 2022 04:42
@marselester
Debug "use of closed network connection" error
cc6e0c0 
Context cancelled, server emitted 2670 signals.
panic: read unix @->/run/user/1001/bus
@marselester marselester force-pushed the reduce-unix-read-allocs branch from 610d8b4 to cc6e0c0 Compare December 6, 2022 04:44
@marselester marselester marked this pull request as draft December 6, 2022 21:39
@marselester marselester force-pushed the reduce-unix-read-allocs branch 3 times, most recently from 35cb2ef to 925ecbb Compare December 7, 2022 05:11
@marselester
Copy link
Contributor Author

marselester commented Dec 7, 2022

I figured out why there was an extra memory consumption per operation after refactoring and resolved the problem.

BenchmarkUnixFDs-2   	    7988	    298127 ns/op	   16989 B/op	     614 allocs/op

@marselester marselester marked this pull request as ready for review December 7, 2022 05:38
@marselester marselester force-pushed the reduce-unix-read-allocs branch from 925ecbb to a8c1d8f Compare December 7, 2022 18:36
@marselester
Copy link
Contributor Author

marselester commented Dec 7, 2022

Down to 606 allocs from 907, also the benchmark shows 8.17KB less memory usage.

# new
BenchmarkUnixFDs-2   	    7674	    237934 ns/op	   16368 B/op	     606 allocs/op
# old
BenchmarkUnixFDs-2   	    6574	    330534 ns/op	   24737 B/op	     907 allocs/op

@marselester
Copy link
Contributor Author

marselester commented Dec 8, 2022

I will try these changes on some realistic workload to make sure there is no regression.

@marselester
Copy link
Contributor Author

marselester commented Dec 12, 2022

I have tested the changes on Parca Agent and posted memory profiles here parca-dev/parca-agent#1037 (comment).

guelfey
guelfey approved these changes Dec 17, 2022
View reviewed changes
Copy link
Member

@guelfey guelfey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the work and detailed stats!

@guelfey guelfey merged commit ec98ef2 into godbus:master Dec 17, 2022
@marselester marselester mentioned this pull request Dec 18, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@guelfey guelfey guelfey approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@marselester @guelfey