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.

Sign up for GitHub

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

Jump to bottom

Remove request allocation backpressure #241

Closed
hannahhoward opened this issue Oct 6, 2021 · 2 comments · Fixed by #272
Closed

Remove request allocation backpressure #241

hannahhoward opened this issue Oct 6, 2021 · 2 comments · Fixed by #272
Assignees
@rvagg
Labels
effort/days Estimated to take multiple days, but less than a week exp/expert Having worked on the specific codebase is important need/triage Needs initial labeling and prioritization P2 Medium: Good to have, but can wait until someone steps up

Comments

@hannahhoward
Copy link
Collaborator

hannahhoward commented Oct 6, 2021 •
edited
Loading

What

Request allocation backpressure was thrown in quite ad-hoc to respond to miner concerns about memory usage. (as opposed to response allocation backpressure which has seen pretty careful development over a while).

I have been aware of potential issues with it for some time, but have failed to properly address them as they require a fair amount of plumbing to get right.

Given the presences of outgoing request limits, it seems prudent to remove allocation backpressure on the requestor side as a first step.

How

  • Remove top level options for configuring allocation backpressure on the Requestor side
  • Remove allocation backpressure from impl graphsync sync receiver
  • Remove allocation backpressure calls from RequestManager
  • Remove allocation backpressure calls from AsyncLoader
  • Remove allocation backpressure test for impl
@hannahhoward hannahhoward added P1 High: Likely tackled by core team if no one steps up need/triage Needs initial labeling and prioritization exp/expert Having worked on the specific codebase is important effort/days Estimated to take multiple days, but less than a week labels Oct 6, 2021
@hannahhoward
Copy link
Collaborator Author

Follow-up: at this point, we have simultaneous outgoing requests limits, where we didn't before. The memory backpressure on the requestor side has been a source of issues and was put in somewhat haphazardly at a time when we didn't have these -- is it neccesary? should it be removed entirely?

@hannahhoward hannahhoward added P2 Medium: Good to have, but can wait until someone steps up and removed P1 High: Likely tackled by core team if no one steps up labels Oct 26, 2021
@hannahhoward hannahhoward moved this from Icebox to Backlog in Project Thunder (Interop) Nov 12, 2021
@hannahhoward hannahhoward moved this from Backlog to Ready in Project Thunder (Interop) Nov 13, 2021
@hannahhoward hannahhoward moved this from Ready to Backlog in Project Thunder (Interop) Nov 13, 2021
@hannahhoward hannahhoward changed the title Architectural cleanup on request allocation backpressure Remove request allocation backpressure Nov 16, 2021
@hannahhoward hannahhoward moved this from Backlog to Ready in Project Thunder (Interop) Nov 16, 2021
@rvagg
Copy link
Member

rvagg commented Nov 17, 2021

I believe this was introduced primarily in 9171ce6

rvagg added a commit that referenced this issue Nov 17, 2021
@rvagg
feat!(requestmanager): remove request allocation backpressure
ae8a908 
Closes: #241
Ref: 9171ce6
@rvagg rvagg mentioned this issue Nov 17, 2021
Merged
5 tasks
@rvagg rvagg moved this from Ready to In Progress in Project Thunder (Interop) Nov 17, 2021
hannahhoward added a commit that referenced this issue Nov 19, 2021
@rvagg @hannahhoward
feat!(requestmanager): remove request allocation backpressure (#272)
623c835 
* feat!(requestmanager): remove request allocation backpressure

Closes: #241
Ref: 9171ce6

* fixup! feat!(requestmanager): remove request allocation backpressure

Co-authored-by: Hannah Howard <[email protected]>
Repository owner moved this from In Progress to Done in Project Thunder (Interop) Nov 19, 2021
rvagg added a commit that referenced this issue Nov 30, 2021
@rvagg @hannahhoward
feat!(requestmanager): remove request allocation backpressure (#272)
ed50782 
* feat!(requestmanager): remove request allocation backpressure

Closes: #241
Ref: 9171ce6

* fixup! feat!(requestmanager): remove request allocation backpressure

Co-authored-by: Hannah Howard <[email protected]>
marten-seemann pushed a commit that referenced this issue Mar 2, 2023
@dirkmc
reduce channel monitor log verbosity (#241)
c5b8330 
* refactor: reduce channel monitor log verbosity

* refactor: debug -> info
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@rvagg rvagg

Labels
effort/days Estimated to take multiple days, but less than a week exp/expert Having worked on the specific codebase is important need/triage Needs initial labeling and prioritization P2 Medium: Good to have, but can wait until someone steps up
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

feat!(requestmanager): remove request allocation backpressure ipfs/go-graphsync
2 participants
@rvagg @hannahhoward