Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

lnpeer+brontide: reduce memory footprint using read/write pools for message encode/decode #2474

Merged
merged 9 commits into from Feb 24, 2019

Conversation

Projects
None yet
3 participants
@cfromknecht
Copy link
Collaborator

commented Jan 15, 2019

This PR is a follow up to #2419 and #2385, introducing an lnpeer.WritePool and brontide.ReadPool. Requests to read/write messages are submitted to the respective pool, which maintains a set of workers that handle the encoding/decoding. This builds on the prior PRs, since each pool is backed by a lnpeer.WriteBufferPool and brontide.ReadBufferPool, ensuring that buffer used in each are efficiently reused before being returned to the runtime.

With #2419 and #2385, the total memory that will be allocated is linear in the number of peers/connections maintained by the daemon. With the read and write pools, we modify that to be at most linear in the number of specified workers. More importantly, these changes completely decouple read and write buffer allocations from the peer/connection lifecycle, allowing LND to tolerate flapping peers with minimal overhead.

Nodes that have a large number of peers will see the most drastic benefit. In testing, I was able to connect (w/o gossip queries) to nearly 900 unique nodes, all while keeping LNDs total memory under 15 MB. This configuration could have easily connected to more nodes, though that was all that reachable via the bootstrapper.

This same test used between 90-100MB on master, and continues to grow as more connections are established. In contrast, the memory used with read/write pools remains constant even as more peers are added.

Note that of the 15MB, only about 1MB is actually from the read/write pools, the remaining overhead is created by other subsystems operating in LND, and is reflected in both measurements. Coupled with more efficient recycling of unused buffers, I'd expect the reduction in memory related to read/write buffers to be one or two orders of magnitude for most nodes.

Depends on:

@cfromknecht cfromknecht force-pushed the cfromknecht:read-and-write-pools branch from 36d4e21 to 68ab99e Jan 15, 2019

Show resolved Hide resolved peer.go

@cfromknecht cfromknecht force-pushed the cfromknecht:read-and-write-pools branch 2 times, most recently from 4b96d7b to 25dbe0b Jan 16, 2019

@Roasbeef Roasbeef added this to the 0.6 milestone Jan 16, 2019

@cfromknecht cfromknecht added the blocked label Jan 16, 2019

@cfromknecht cfromknecht force-pushed the cfromknecht:read-and-write-pools branch 2 times, most recently from 8af29b1 to 8d90d78 Feb 6, 2019

@wpaulino wpaulino referenced this pull request Feb 15, 2019

Merged

brontide: read buffer pool #2419

1 of 1 task complete
@Roasbeef

This comment has been minimized.

Copy link
Member

commented Feb 19, 2019

All the dependent PRs have been merged now, so this needs a rebase!

Show resolved Hide resolved lnpeer/write_pool.go Outdated

// DefaultWorkerTimeout is the default interval after which a worker will return
// it's write buffer to the buffer pool if no write requests are submitted.
const DefaultWorkerTimeout = 5 * time.Second

This comment has been minimized.

Copy link
@Roasbeef

Roasbeef Feb 19, 2019

Member

Is there any interplay with this value and the internal GC queue timeout values?

This comment has been minimized.

Copy link
@cfromknecht

cfromknecht Feb 21, 2019

Author Collaborator

not really, this dictates how long we will hold on to buffers taken from the pool if no new tasks are received, the other is how long we'll keep the returned buffers in the pool if no new workers are spawned. they can be fairly independent, though probably some room for tuning

Show resolved Hide resolved lnpeer/write_pool.go Outdated
Show resolved Hide resolved lnpeer/write_pool.go Outdated
Show resolved Hide resolved lnpeer/write_pool.go Outdated
Show resolved Hide resolved peer.go
Show resolved Hide resolved peer.go
Show resolved Hide resolved server.go
Show resolved Hide resolved brontide/read_pool.go Outdated

@cfromknecht cfromknecht force-pushed the cfromknecht:read-and-write-pools branch 3 times, most recently from 33cb84c to de99723 Feb 20, 2019

@cfromknecht cfromknecht removed the blocked label Feb 21, 2019

@cfromknecht

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 21, 2019

@Roasbeef pool.Read and pool.Write now reside in the pool package, lmk how you like the new structure. some lingering godocs left to finish in the new unit tests, but o/w should be ready to another review, ptal

@cfromknecht cfromknecht force-pushed the cfromknecht:read-and-write-pools branch 2 times, most recently from 2e7f276 to dffb82f Feb 22, 2019

@Roasbeef
Copy link
Member

left a comment

Excellent work with the code consolidation between this diff and the prior one. However, I think it's a mistake to force the brontide layer to be aware of the usual net.Conn implementation of the io.Reader it see. More rationale has been provided in line.

Show resolved Hide resolved pool/worker.go
Show resolved Hide resolved pool/write.go
Show resolved Hide resolved pool/read.go
Show resolved Hide resolved brontide/noise.go Outdated
Show resolved Hide resolved brontide/noise.go Outdated
@@ -743,7 +748,19 @@ func (b *Machine) WriteMessage(w io.Writer, p []byte) error {
// ReadMessage attempts to read the next message from the passed io.Reader. In
// the case of an authentication error, a non-nil error is returned.
func (b *Machine) ReadMessage(r io.Reader) ([]byte, error) {
_, err := io.ReadFull(r, b.nextCipherHeader[:])
conn, ok := r.(net.Conn)

This comment has been minimized.

Copy link
@Roasbeef

Roasbeef Feb 22, 2019

Member

This leaks the concrete implementation of the io.Reader. What's the rationale of pushing the deadline iteration down into the brontide layer rather than keeping it one step above at the peer?

This comment has been minimized.

Copy link
@cfromknecht

cfromknecht Feb 22, 2019

Author Collaborator

the abstraction is already somewhat broken imo bc it doesn't distinguish between adversarial vs non-adversarial io.Reader instances, e.g. connections vs files. I considered adding a separate ReadConn method to handle this

setting the timeout at the peer level will likely cause reads to timeout while the read is being scheduled, since it is unaware of when the task is being scheduled by the pool.

there may also be anan arbitrary amount of time between reading payload lengths (which we want to block for), though the header is received we expect the body of the message to follow shortly after. this was also less ugly than having to have something like this in the peer:

retry:
conn.SetReadDeadline(deadline)
_, err := conn.ReadMessage()
if nerr := err.(net.Error); nerr.Timeout() {
     goto retry
}

as this would mean the read timeout will be triggered multiple times if the peer doesn't send us anything for a minute and we want the read timeout to be something short like 5s

This comment has been minimized.

Copy link
@cfromknecht

cfromknecht Feb 22, 2019

Author Collaborator

another consequence of setting the read deadline at the peer level is that we may successfully read the message header, but then timeout before/while reading the body. in this case, and w/ the retry logic, we will try again and start reading a body as if it were the header since we aren't distinguishing timeouts on header vs body

This comment has been minimized.

Copy link
@cfromknecht

cfromknecht Feb 22, 2019

Author Collaborator

we can push the read timeout abstraction up a bit by splitting ReadMessage into ReadHeader and ReadBody. this way the caller can block arbitrarily long on headers, and then set a real deadline for bodies. because of the scheduling tho, it would still be more prone to timing out while being scheduled than if the timeout is set once the task is scheduled

I think we could get around this a little more by having ReadMessage accept the buffer we want to read into, then from the peer we can do:

conn.SetReadDeadline(time.Time{})
conn.ReadHeader()
readPool.Submit(func(b *buffer.Read) error {
    conn.SetReadDeadline(time.Now().Add(readTimeout)
    rawBytes, err := conn.ReadNextBody(b) // internally calls Noise.ReadMessage(conn)
    return err
})

how does that sound? think this is the best alternative i've seen that doesn't end up with nuanced timeout cases when trying to use a unified ReadMessage and gets rid of all notions of timeouts handling from the brontide pkg

This comment has been minimized.

Copy link
@cfromknecht

cfromknecht Feb 22, 2019

Author Collaborator

i think this also allows us to keep ReadMessage and note that it should only be used for non-adverserial readers, and if the caller needs more control they should use the split header/body interface. interested in hearing your thoughts

This comment has been minimized.

Copy link
@Roasbeef

Roasbeef Feb 24, 2019

Member

Nice, I think that last fragment gives us the best of both worlds. Also nice to patch the issue that exists right now that allows a peer to block us by writing the brontide header and nothing else.

Re the first comment, I don't think it's the case that the abstraction is broken if it doesn't distinguish between sockets, files, etc. On the contrary, that's the point of having something like an io.Reader in the first place: we don't care what the underlying implementation is.

Show resolved Hide resolved brontide/noise.go Outdated
Show resolved Hide resolved peer.go

@cfromknecht cfromknecht force-pushed the cfromknecht:read-and-write-pools branch from dffb82f to 02ea501 Feb 22, 2019

@cfromknecht

This comment has been minimized.

Copy link
Collaborator Author

commented Feb 22, 2019

@Roasbeef i've added some commits that implement a PoC split ReadNextHeader/ReadNextBody, while still retaining the unified ReadMessage method for non-adversarial cases. I must say, I much prefer this approach as it pushes all timeout handling into the peer and keeps the brontide package much simpler :)

cfromknecht added some commits Feb 22, 2019

cfromknecht added some commits Feb 22, 2019

@cfromknecht cfromknecht force-pushed the cfromknecht:read-and-write-pools branch from 4e3780a to db2c104 Feb 22, 2019

@Roasbeef
Copy link
Member

left a comment

LGTM 🐲

Super happy with the way this turned out in the end! Also nice to explicitly handle the adversarial cases introduces by using a raw net.Conn within the peer itself, rather than burdening the brontide.Machine with timeout awareness. Once go 1.12 drops we should seed an additional reduction in memory usage as it'll start to return memory to the OS more aggressively than it does atm. On most of my profiles, I see a very small amount of active heap usage (reported directly by the runtime) leading me to believe there's a good chunk of bloat just sitting around waiting to be returned to the OS. Once we implement the query sync spot checks (along with the message prio in the queueHandler), starting up lnd should no longer cause such a massive memory+cpu burst,

@Roasbeef Roasbeef merged commit a6ba965 into lightningnetwork:master Feb 24, 2019

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
coverage/coveralls First build on read-and-write-pools at 60.177%
Details
@capacitynetwork

This comment has been minimized.

Copy link

commented Mar 4, 2019

After experiencing #2726 I have changed writeMessageTimeout to 25 on my node and it appears to have helped.

@cfromknecht was there a reason for change from 50 to 10?

@cfromknecht cfromknecht deleted the cfromknecht:read-and-write-pools branch Mar 4, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.