Opened 3 weeks ago

Closed 13 days ago

#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:

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.

Subtickets

Change History (10)

comment:1 Changed 3 weeks ago by jogger

  • Description modified (diff)

comment:2 Changed 3 weeks ago by zab

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 3 weeks 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 3 weeks ago by zab

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 3 weeks 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 weeks 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 weeks 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 weeks 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 weeks ago by jogger

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

comment:10 Changed 13 days ago by zzz

  • Milestone changed from undecided to n/a
  • Resolution set to duplicate
  • Status changed from new to closed

Closed at OP's request, see #2427

Note: See TracTickets for help on using tickets.