[jitsi-dev] [jitsi/libjitsi] OveruseEstimator replace LinkedList<Double> with double[] (#130)


#1

this avoid a lot of autoboxing, and is more efficient

Signed-off-by: Etienne CHAMPETIER <champetier.etienne@gmail.com>
You can view, comment on, or merge this pull request online at:

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

-- Commit Summary --

  * OveruseEstimator replace LinkedList<Double> with double[]

-- File Changes --

    M src/org/jitsi/impl/neomedia/rtp/remotebitrateestimator/OveruseEstimator.java (25)
    M src/org/jitsi/impl/neomedia/transform/srtp/BaseSRTPCryptoContext.java (2)

-- Patch Links --

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

···

---
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/libjitsi/pull/130


#2

@@ -268,7 +268,7 @@ protected BaseSRTPCryptoContext(
      * @param pkt the RTP packet to be authenticated
      * @param rocIn Roll-Over-Counter
      */
- synchronized protected void authenticatePacketHMAC(RawPacket pkt, int rocIn)
+ protected void authenticatePacketHMAC(RawPacket pkt, int rocIn)

How is this related to the subject of this PR and/or OveruseEstimator? @champtar, could I please ask you to keep PRs to a single, standalone subject? If you need to work on two dependent subjects, you can still create two PRs and state in one of them that it depends on the other and paste a link to it.

···

---
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/libjitsi/pull/130/files/8d7776133fc2780eddd1f3f596253928b7c99372#r58969377


#3

Jenkins, test this please.

···

---
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/libjitsi/pull/130#issuecomment-207202938


#4

@@ -268,7 +268,7 @@ protected BaseSRTPCryptoContext(
      * @param pkt the RTP packet to be authenticated
      * @param rocIn Roll-Over-Counter
      */
- synchronized protected void authenticatePacketHMAC(RawPacket pkt, int rocIn)
+ protected void authenticatePacketHMAC(RawPacket pkt, int rocIn)

This should definitly not be here (git commit -a ...), thanks for the catch, I'll remove it

···

---
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/libjitsi/pull/130/files/8d7776133fc2780eddd1f3f596253928b7c99372#r58980447


#5

I've removed the intruder :slight_smile:

···

---
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/libjitsi/pull/130#issuecomment-207248777


#6

@@ -86,7 +86,8 @@

     private double slope;

- private final List<Double> tsDeltaHist = new LinkedList<>();
+ private final double[] tsDeltaHist = new double[kMinFramePeriodHistoryLength];
+ private int tsDeltaHistIdx = 0;

As you've probably figured out based on the sparse class comment that I've put, OveruseDetector was translated from C++. Consequently, it lacks javadocs where there no equivalents in the respective C++ source code.

However, we need comments where we author source code. And in your case, I'd say that javadocs are pretty necessary because you're practically implementing a circular buffer and that's complex enough to not be immediately recognizable. Without good javadocs, I'd say the supposed performance improvement is not big enough to justify modifying a translated piece.

···

---
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/libjitsi/pull/130/files/fb76c567c1ce55d8440ddcc3f03973213daa7b4a#r59084559


#7

@@ -99,6 +100,8 @@ public OveruseEstimator(OverUseDetectorOptions options)
         varNoise = options.initialVarNoise;
         E = clone(options.initialE);
         processNoise = options.initialProcessNoise.clone();
+ //init with max value to simplify updateMinFramePeriod

I don't like such formatting of comments. Make it a proper sentence in written English please.

···

---
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/libjitsi/pull/130/files/fb76c567c1ce55d8440ddcc3f03973213daa7b4a#r59084829


#8

@@ -86,7 +86,8 @@

     private double slope;

- private final List<Double> tsDeltaHist = new LinkedList<>();
+ private final double[] tsDeltaHist = new double[kMinFramePeriodHistoryLength];
+ private int tsDeltaHistIdx = 0;

I think the part Idx of the name tsDeltaHistIdx could be more meaningful. Index is generic enough to not bring much but the fact that it's an index (because the fact that it's related to the array tsDeltaHist is represented by the prefix tsDeltaHist). If I'm reading your source code right, you're implementing a circular buffer and tsDeltaHistIdx is the tail (or the insertion head but that's another discussion), Wouldn't it be better to reflect more meaning in the name?

···

---
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/libjitsi/pull/130/files/fb76c567c1ce55d8440ddcc3f03973213daa7b4a#r59086726


#9

@@ -229,14 +232,18 @@ public void update(

     private double updateMinFramePeriod(double tsDelta)
     {
- double minFramePeriod = tsDelta;
-
- if (tsDeltaHist.size() >= kMinFramePeriodHistoryLength)
- tsDeltaHist.remove(0);
- for (Double d : tsDeltaHist)
- minFramePeriod = Math.min(d, minFramePeriod);
- tsDeltaHist.add(tsDelta);
- return minFramePeriod;
+ tsDeltaHist[tsDeltaHistIdx] = tsDelta;
+ tsDeltaHistIdx = (tsDeltaHistIdx + 1) % tsDeltaHist.length;

Is that (much) faster than the following? I'm curious if you have the answer (i.e. I don't want you to research it or change your source code).

int nextTsDeltaHistIdx = tsDeltaHistIdx + 1;
tsDeltaHistIdx = (nextTsDeltaHistIdx < tsDeltaHist.length) ? nextTsDeltaHistIdx : 0;
···

---
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/libjitsi/pull/130/files/fb76c567c1ce55d8440ddcc3f03973213daa7b4a#r59087681


#10

@@ -229,14 +232,18 @@ public void update(

     private double updateMinFramePeriod(double tsDelta)
     {
- double minFramePeriod = tsDelta;
-
- if (tsDeltaHist.size() >= kMinFramePeriodHistoryLength)
- tsDeltaHist.remove(0);
- for (Double d : tsDeltaHist)
- minFramePeriod = Math.min(d, minFramePeriod);
- tsDeltaHist.add(tsDelta);
- return minFramePeriod;
+ tsDeltaHist[tsDeltaHistIdx] = tsDelta;
+ tsDeltaHistIdx = (tsDeltaHistIdx + 1) % tsDeltaHist.length;

i've no idea, it's just how it went out of my brain :slight_smile:

···

---
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/libjitsi/pull/130/files/fb76c567c1ce55d8440ddcc3f03973213daa7b4a#r59169113


#11

hi @lyubomir
i've updated the commit with more javadocs

···

---
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/libjitsi/pull/130#issuecomment-208234613


#12

here is the top 20 functions while profiling JVB:

	(t  9,2,s  9,1) java.lang.Object.wait
	(t  5,8,s  5,8) java.net.PlainDatagramSocketImpl.send
	(t  4,9,s  4,9) java.net.PlainDatagramSocketImpl.receive0
	(t  2,7,s  2,7) sun.misc.Unsafe.park
	(t  3,0,s  2,3) java.util.concurrent.locks.AbstractQueuedSynchronizer.release
	(t  2,0,s  2,0) org.jitsi.impl.neomedia.transform.srtp.OpenSSLHMAC.HMAC_Update
	(t  1,7,s  1,7) java.lang.Object.notifyAll
	(t  1,7,s  1,6) java.util.concurrent.ArrayBlockingQueue.enqueue
	(t  1,5,s  1,5) org.jitsi.impl.neomedia.transform.srtp.SRTPCipherCTROpenSSL.AES128CTR_CTX_process
	(t  2,2,s  1,4) org.ice4j.socket.MultiplexingXXXSocketSupport.acceptBySocketsOrThis
	(t  1,3,s  1,3) sun.misc.Unsafe.unpark
	(t  1,3,s  1,0) org.jitsi.impl.neomedia.RTPConnectorInputStream.read
	(t  1,0,s  1,0) java.util.HashMap.getNode
	(t  1,0,s  1,0) java.lang.Double.valueOf
	(t  0,8,s  0,8) org.jitsi.impl.neomedia.transform.srtp.OpenSSLHMAC.HMAC_Final
	(t  8,4,s  0,7) org.ice4j.socket.MultiplexingXXXSocketSupport.receive
	(t  0,7,s  0,7) java.lang.Thread.setDaemon
	(t  0,6,s  0,6) java.lang.Object.notify
	(t  8,8,s  0,6) org.ice4j.ice.harvest.SinglePortUdpHarvester.runInHarvesterThread
	(t  0,6,s  0,6) java.util.concurrent.ConcurrentHashMap.get

```java.lang.Double.valueOf``` is 14th and using 2/3 the time used by ```AES128CTR_CTX_process``` (1,0 vs 1,5) so this isn't a major performance improvements but still is good to take

···

---
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/libjitsi/pull/130#issuecomment-208245668


#13

Merged #130.

···

---
You are receiving this because you are subscribed to this thread.
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/libjitsi/pull/130#event-654240338