[jitsi-dev] [libjitsi] Config cache (#69)


#1

This pull requests makes nack cache settings configurable out of .sip-communicator.properties. Settings include:
    -Max number of packets
    -Number of streams to cache
    -Max milliseconds of data to
You can view, comment on, or merge this pull request online at:

  https://github.com/jitsi/libjitsi/pull/69

-- Commit Summary --

  * Using ConfigurationService to read caching properties from config file
  * Testing

-- File Changes --

    M src/org/jitsi/impl/neomedia/transform/CachingTransformer.java (32)

-- Patch Links --

https://github.com/jitsi/libjitsi/pull/69.patch
https://github.com/jitsi/libjitsi/pull/69.diff

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/libjitsi/pull/69


#2

Thank you for the contribution! I left a couple of notes on the commit page. Before we merge, we need you to sign our CLA ([individual](https://jitsi.org/icla) or [corporate](https://jitsi.org/ccla)).

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/libjitsi/pull/69#issuecomment-165261664


#3

No problem! I've made the requested changes.

Also, it looks like my team has signed the corporate CLA, but we're not quite sure how to add me to it? Do you have any insight there?

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/libjitsi/pull/69#issuecomment-165503047


#4

Merged #69.

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/libjitsi/pull/69#event-496072157


#5

Thanks!
About the CLA: no action needed on your end, we added you to the existing one.

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/libjitsi/pull/69#issuecomment-165849957


#6

BTW, we would be interested to hear if you have any observations with regard to the optimal sizes for the cache, in order to improve the default values.

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/libjitsi/pull/69#issuecomment-165850456


#7

Yeah for sure. We definitely wanted to make this configurable as some of the behavior we were seeing might be specific to our wireless network. Also, the values we're using now are significantly higher than the current defaults. Probably high enough to be considered controversial.

We were seeing (on fairly lossy wifi) NACKs come in >2 seconds after packet loss was detected. The missing packets are obviously irrelevant that much later, but we found that if the NACKs were propagated back to the sender it would cause a dramatic drop in it's send rate. This was causing artificially low performance especially with wired participants that were able to download these "missing" packets without issue.

We set our cache to keep packets for 10 seconds (~3000 packets) to allow for cache hits on these late NACKs. We've seen surprisingly impressive improvements with these values in production. We haven't seen any issues with system resources yet, but we're keeping an eye on it.

The values we're using now are:

  org.jitsi.impl.neomedia.transform.CachingTransformer.CACHE_SIZE_MILLIS=10000
  org.jitsi.impl.neomedia.transform.CachingTransformer.CACHE_SIZE_PACKETS=3000
  org.jitsi.impl.neomedia.transform.CachingTransformer.CACHE_SIZE_STREAMS=10

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/libjitsi/pull/69#issuecomment-165909963