[jitsi-dev] [jitsi-videobridge] Added use of Atomics for expired and lastActivityTime (#48)


#1

The use of Atomics to replace synchronized blocks prevents thread contention and possible deadlocking. Atomics use CAS and are guaranteed to be atomic. In addition this style is also known to speed things up in a lot of instances.
http://www.ibm.com/developerworks/library/j-jtp11234/
You can merge this Pull Request by running:

  git pull https://github.com/mondain/jitsi-videobridge patch-6

Or you can view, comment on it, or merge it online at:

  https://github.com/jitsi/jitsi-videobridge/pull/48

-- Commit Summary --

  * Added use of Atomics for expired and lastActivityTime

-- File Changes --

    M src/org/jitsi/videobridge/Channel.java (34)

-- Patch Links --

https://github.com/jitsi/jitsi-videobridge/pull/48.patch
https://github.com/jitsi/jitsi-videobridge/pull/48.diff

···

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


#2

The following code is NOT thread safe:

    if (getLastActivityTime() < now)
        lastActivityTime.set(now);

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi-videobridge/pull/48#issuecomment-65827489


#3

While not absolutely "thread-safe" per se, it does not incur a possible deadlock as the replaced version does; in addition, the lastActivityTime will always be the latest "now" since the variable itself is atomic.

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi-videobridge/pull/48#issuecomment-65841336


#4

While not absolutely "thread-safe" per se, it does not incur a possible deadlock as the replaced version
does;

The fact that there is a possibility for a deadlock based only on the fact that a single monitor is used will not make me remove the thread safety. A deadlock requires the involvement of a least two resources.

in addition, the lastActivityTime will always be the latest "now" since the variable itself is atomic.

No, the two lines are not atomic as a whole just because each of them is atomic on its own.

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi-videobridge/pull/48#issuecomment-65846452


#5

Correct me if I'm wrong, but in the original code there is a single monitor accessed in multiple places in this class. Keeping the "safety" and possible deadlock in-place for lastActivityTime is fine by me, but my replacement for expired is solid and more efficient. As an aside on lastActivityTime, exactly how many threads are accessing it at any one time? My assumption (which could be wrong) is 1 per Channel.

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi-videobridge/pull/48#issuecomment-65847942


#6

Correct me if I'm wrong, but in the original code there is a single monitor accessed in multiple places in this class.

It's still a single monitor. It is likely that there is a second monitor which could cause a deadlock in combination with this monitor but you haven't mentioned it yet.

but my replacement for expired is solid and more efficient.

Yes, I like the atomic expired at this point and it's the only reason I haven't closed this pull request with a rejection yet. Unfortunately, you chose to couple two separate patches into one.

As an aside on lastActivityTime, exactly how many threads are accessing it at any one time? My assumption (which could be wrong) is 1 per Channel.

A channel could be "touched" from multiple threads simultaneously e.g. COLIBRI, ICE, RTP.

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi-videobridge/pull/48#issuecomment-65849953


#7

Good info on touch(), thanks; also I removed the lastActivityTime code.

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi-videobridge/pull/48#issuecomment-65855721


#8

If there's an actual concern about getting into a deadlock, then doing a spin-loop with compareAndSet for the lastActivityTime might be an option, e.g. like the following snippet. But please open a separate pull request for that.

private AtomicLong lastActivityTime = new AtomicLong();
//...
public void touch()
{
    long now = System.currentTimeMillis();
    while (true) {
        int cur = lastActivityTime.get();
        if (now > cur)
        {
            if (lastActivityTime.compareAndSet(cur, now))
            {
                return true;
            }
        }
        else
        {
            return false;
        }
    }
}
···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi-videobridge/pull/48#issuecomment-65891061


#9

I didn't think you'd be up for looping there, I've got a similar block of code that essentially does the same thing:

    long now = System.currentTimeMillis();
    while (true) {
        long stored = lastActivityTime.get();
        if (stored >= now) break;
        if (lastActivityTime.compareAndSet(stored, now)) break;
    }
···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi-videobridge/pull/48#issuecomment-65900799


#10

Can one of the admins verify this patch?

···

---
Reply to this email directly or view it on GitHub:
https://github.com/jitsi/jitsi-videobridge/pull/48#issuecomment-192016176


#11

This code has been refactored quite a lot now

···

--
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/jitsi-videobridge/pull/48#issuecomment-343326824


#12

Closed #48.

···

--
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/jitsi-videobridge/pull/48#event-1335117302