Opened 7 years ago

Closed 5 years ago

#786 closed enhancement (fixed)

Move stats updating out of codel()

Reported by: Zlatin Balevsky Owned by: zzz
Priority: trivial Milestone: 0.9.4
Component: router/transport Version: 0.9.3
Keywords: codel contention stats Cc: zab@…
Parent Tickets: Sensitive: no

Description

While profiling some critical paths I observed some situations where updating the stats from inside the codel() method was a bottleneck. The full stack trace is available http://pastethis.i2p/show/2285/

That trace was taken with yourkit while allocation recording was enabled which adds some overhead, so in production the contention is not that bad. Nevertheless it should be very easy to move the RateStat.addData(..) outside of the lock.

Subtickets

Change History (7)

comment:1 Changed 7 years ago by zzz

We could also make them non-required stats (at least the delay ones), so there's almost no overhead.

comment:2 Changed 7 years ago by Zlatin Balevsky

Cc: zab@… added

naive approach http://pastethis.i2p/show/2288/

I see it will miss a stat update if it drops because the _lastSojurn will be overwritten by the dequeueing of the following packet; don't know how much of a problem that is but it can be worked around with an extra field and a flag.

comment:3 Changed 7 years ago by zzz

I wouldn't mess with the drop stat. First of all it doesn't happen very often. Secondly, there can be multiple drops in the while loop inside codel(). We really want an accurate event count and avg delay of the drops, we don't want to consolidate them into one addRate() call.

For the delay stat, seems easiest to just make it a non-required stat and call it a day.

comment:4 Changed 7 years ago by Zlatin Balevsky

Don't feel strongly either way, and maybe #787 will make this a non-issue. Will do some profiling and find out

comment:5 in reply to:  4 Changed 7 years ago by Zlatin Balevsky

Replying to zab:
Profiling confirmed that if the patches suggested in #787 are applied this ticket is no longer an issue.

comment:6 Changed 7 years ago by zzz

I'm going to make all the codel stats non-required anyway.

comment:7 Changed 5 years ago by zzz

Milestone: 0.9.4
Resolution: fixed
Status: newclosed

Non-required in 9fc2cf8b25a868d01752bc39b394d263c36bebd2 released in 0.9.4. #787 is also closed.

Note: See TracTickets for help on using tickets.