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

webtransport: use the rcmgr to control flow control window increases #1832

Merged
merged 2 commits into from
Oct 24, 2022

Conversation

marten-seemann
Copy link
Contributor

@marten-seemann marten-seemann commented Oct 17, 2022

Fixes #1826.

Depends on quic-go/webtransport-go#35.

@marten-seemann marten-seemann force-pushed the webtransport-rcmgr branch 4 times, most recently from b421e7f to 0eb8dbd Compare October 17, 2022 10:51
p2p/transport/quic/tracer_metrics.go Outdated Show resolved Hide resolved
@@ -60,7 +60,18 @@ func (c *conn) AcceptStream() (network.MuxedStream, error) {
return &stream{str}, nil
}

func (c *conn) Close() error { return c.session.Close() }
func (c *conn) allowWindowIncrease(size uint64) bool {
return c.scope.ReserveMemory(int(size), network.ReservationPriorityMedium) == nil
Copy link
Collaborator

Choose a reason for hiding this comment

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

What's the argument for Medium vs High? Medium bounds us to 60% of available memory. So a 4GiB node with defaults would have 0.6(512+128MiB)=384MiB here. I think that sounds fine, just want to check though

Copy link
Contributor Author

Choose a reason for hiding this comment

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

When we're running low on memory, we want to block allocations that are not strictly necessary. Performance will be slightly degraded if you don't get the full receive window, but things will still work. Whereas if you fail to allocate the buffer needed for parsing a protobuf (for example), things will break in a lot worse ways.


proxy, err := quicproxy.NewQuicProxy("localhost:0", &quicproxy.Opts{
RemoteAddr: ln.Addr().String(),
DelayPacket: func(quicproxy.Direction, []byte) time.Duration { return rtt / 2 },
Copy link
Collaborator

Choose a reason for hiding this comment

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

Cool! TIL

for {
_, err := str.Write(bytes.Repeat([]byte{0x42}, 1<<10))
require.NoError(t, err)
if atomic.LoadUint32(&increasesDone) > 0 {
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: maybe easier to use a select over a done semaphore?

Suggested change
if atomic.LoadUint32(&increasesDone) > 0 {
select {
case <-increasesDone:
return
default:
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This will be nicer once we can use the atomic.Bool that was introduced in Go 1.19: https://pkg.go.dev/sync/atomic#Bool.

@marten-seemann marten-seemann merged commit c33f910 into master Oct 24, 2022
@marten-seemann marten-seemann mentioned this pull request Oct 31, 2022
34 tasks
@marten-seemann marten-seemann deleted the webtransport-rcmgr branch November 8, 2022 13:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

webtransport: use the resource manager to control flow control window increases
3 participants