Opened 2 years ago

Closed 2 years ago

#2424 closed defect (duplicate)

_sendBps overstated in

Reported by: jogger Owned by: zzz
Priority: minor Milestone: n/a
Component: router/transport Version: 0.9.38
Keywords: Cc:
Parent Tickets: Sensitive: no

Description (last modified by jogger)

Bytes sent are double counted when the send window is rolled. Might negatively affect the b/w limiter, especially when on low b/w.

I have also removed excessive flow control and straightened out use of now(). Patch to follow after test.


Change History (10)

comment:1 Changed 2 years ago by jogger

Description: modified (diff)

comment:2 Changed 2 years ago by Zlatin Balevsky

I don't immediately see how it's overstated, but the only place _sendBps is used is in the "udp.sendBps" metric.. which is not used for any decision making anywhere.

comment:3 Changed 2 years ago by jogger

Look at allocateSendingBytes(). size parameter is twice added when interval is rolled over. And size runs against sendWindowBytes, could have some effect.

comment:4 Changed 2 years ago by Zlatin Balevsky

It's added twice to _sendBytes, but that field is zeroed-out when the window is rolled over. Or am I missing something?

comment:5 Changed 2 years ago by jogger

Yep, so size is going twice into sendBps, into the old interval as well as into the new one. But the effect is only cosmetic, going into some debug statement only. Will post the patch anyway, as it cleans up some spots.

comment:6 Changed 2 years ago by zzz

@OP I'm about to turn my attention to this ticket. Let me know if you're going to provide a patch (or if you aren't)

comment:7 Changed 2 years ago by jogger

@zzz good you ask: This overlaps with #2427 where I have provided a patch. This one here turned out to be cosmetic, for full stats only.

Going over it, after introducing the sliding window over there, now there is a glitch. I will comment #2427.

comment:8 Changed 2 years ago by zzz

OK. zab is feeding me tickets to review, this one was on the list, and #2427 was not, and wasn't referenced here either.

So I still need an answer to my question in comment 6, even if this ticket is "cosmetic":

Are you going to be providing a patch, or should we be doing it, or should this ticket simply be closed without change, or are the changes covered in #2427 ?

comment:9 Changed 2 years ago by jogger

Changes covered in #2427. Can be closed or made a subticket.

comment:10 Changed 2 years ago by zzz

Milestone: undecidedn/a
Resolution: duplicate
Status: newclosed

Closed at OP's request, see #2427

Note: See TracTickets for help on using tickets.