#2424 closed defect (duplicate)

_sendBps overstated in PeerState.java

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 20 months ago by jogger

Description: modified (diff)

comment:2 Changed 20 months 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 20 months 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 20 months 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 20 months 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 20 months 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 20 months 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 20 months 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 20 months ago by jogger

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

comment:10 Changed 20 months 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.