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

<Future as std::..::Future>::poll() always allocates, growing until we're woken #2874

Closed
TheBlueMatt opened this issue Feb 5, 2024 · 1 comment · Fixed by #2894
Closed
Milestone

Comments

@TheBlueMatt
Copy link
Collaborator

<Future as std::..::Future>::poll(), if state.complete is not set, always pushes the provided waker onto the callbacks vec. This means if we get poll'd regularly our memory will grow and grow until we get woken, which may be quite a while. Thus, this may end up using a very nontrivial amount of memory, at least on busy nodes.

@TheBlueMatt TheBlueMatt added this to the 0.0.122 milestone Feb 5, 2024
@nate-lightspark
Copy link

nate-lightspark commented Feb 6, 2024

To add more color to the issue, carrying notes from the Discord thread:

Looking at the memory profiler run with dhat leads to the result in the screenshot. The hot piece looks to be chain_monitor.get_update_future() :

Profiler Result 1

Profiler After

It is an adapted from this example code

The code block is like this for the severe memory leak: Gist 1

Change is like this, seems to be resolving the issue (or makes it less severe): Gist 2

TheBlueMatt added a commit to TheBlueMatt/rust-lightning that referenced this issue Feb 13, 2024
When an `std::future::Future` is `poll()`ed, we're only supposed to
use the latest `Waker` provided. However, we currently push an
`StdWaker` onto our callback list every time `poll` is called,
waking every `Waker` but also using more and more memory until the
`Future` itself is woken.

Here we fix this by removing any `StdWaker`s stored for a given
`Future` when it is `drop`ped or prior to pushing a new `StdWaker`
onto the list when `poll`ed.

Sadly, the introduction of a `Drop` impl for `Future` means we
can't trivially destructure the struct any longer, causing a few
methods to need to take `Future`s by reference rather than
ownership and `clone` a few `Arc`s.

Fixes lightningdevkit#2874
TheBlueMatt added a commit to TheBlueMatt/rust-lightning that referenced this issue Feb 15, 2024
When an `std::future::Future` is `poll()`ed, we're only supposed to
use the latest `Waker` provided. However, we currently push an
`StdWaker` onto our callback list every time `poll` is called,
waking every `Waker` but also using more and more memory until the
`Future` itself is woken.

Here we fix this by removing any `StdWaker`s stored for a given
`Future` when it is `drop`ped or prior to pushing a new `StdWaker`
onto the list when `poll`ed.

Sadly, the introduction of a `Drop` impl for `Future` means we
can't trivially destructure the struct any longer, causing a few
methods to need to take `Future`s by reference rather than
ownership and `clone` a few `Arc`s.

Fixes lightningdevkit#2874
TheBlueMatt added a commit to TheBlueMatt/rust-lightning that referenced this issue Feb 15, 2024
When an `std::future::Future` is `poll()`ed, we're only supposed to
use the latest `Waker` provided. However, we currently push an
`StdWaker` onto our callback list every time `poll` is called,
waking every `Waker` but also using more and more memory until the
`Future` itself is woken.

Here we fix this by removing any `StdWaker`s stored for a given
`Future` when it is `drop`ped or prior to pushing a new `StdWaker`
onto the list when `poll`ed.

Sadly, the introduction of a `Drop` impl for `Future` means we
can't trivially destructure the struct any longer, causing a few
methods to need to take `Future`s by reference rather than
ownership and `clone` a few `Arc`s.

Fixes lightningdevkit#2874
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants