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

Update graphsync & fix in-progress request memory leak by consuming responses #109

Merged
merged 3 commits into from
Oct 29, 2020

Conversation

hannahhoward
Copy link
Collaborator

@hannahhoward hannahhoward commented Oct 28, 2020

Goals

Resolve major in process request memory usage by updating graphsync (work done in repo) and consuming in progress graphsync response channel.

Implementation

  • Update graphsync to v0.4.1 -- there was an issue with backpressure that resulted in a large accumulation of memory on the responding side of graphsync requests. This has now been resolved.
  • Add consuming of response channel to consumeResponses method on Graphsync transport -- the problem here is that graphsync intentionally buffers this channel for you, so you can consume it at your leisure without locking the graphsync implementation. The problem is if you done't consume the response channel the buffer accumulates till the request completes. For a very large request this can be catastrophic. There's possibly a potential design issue here, but for the time being the simple solution is to consume the channel.

For discussion

Here are heap profiles of in use memory objects assembled by running:

go test -v -bench=BenchmarkRoundtripSuccess/test-p2p-stress-1-1GB-no-raw-nodes$ --memprofile=profile.out

Essentially, we're running a 1GB data transfer over a connection gated at 16MB per second -- it will stop in the middle after 10 seconds, and we take the heap profile here.

First, with graphsync v0.3.1:

image

Note the two areas in red -- the one on the left is memory use on the responder side and the one on the right is memory use on the requestor side.

Now, with graphsync v0.4.1:

image

The responder side memory usage is massively reduced (the worst issue) but we still have accumulated data on the requestor side.

Now, adding consuming the response channel in go-data-transfer:

image

The requestor side memory usage is gone.

lastError = err
}
for range responseChan {
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

@hannahhoward Don't we need to do anything with the responses here ? Who are these responses intended for ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's another way to read response data -- in direct IPLD node format. They are still saved to the blockstore anyway so we don't need to do anything with them

Copy link
Collaborator

@aarshkshah1992 aarshkshah1992 left a comment

Choose a reason for hiding this comment

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

LGTM module one question.

@hannahhoward hannahhoward force-pushed the feat/update-graphsync-consume-responses branch from bd002f1 to e528b42 Compare October 28, 2020 18:45
@hannahhoward hannahhoward changed the base branch from feat/benchmarking to master October 28, 2020 18:45
consume response channel so graphsync does not buffer responses in memory
@hannahhoward hannahhoward force-pushed the feat/update-graphsync-consume-responses branch from e528b42 to 4188549 Compare October 29, 2020 02:18
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 this pull request may close these issues.

2 participants