-
Notifications
You must be signed in to change notification settings - Fork 979
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
If the number of handlers is large, split Conference.sendOut on a task queue. #1062
base: master
Are you sure you want to change the base?
Changes from all commits
5f5a9ba
4275c68
4d0cd2a
af1b6da
6a96189
b50e55a
0abeee2
b51be62
b9646b7
7422b4b
4661414
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -386,7 +386,7 @@ public double getRtt() | |
* {@inheritDoc} | ||
*/ | ||
@Override | ||
public boolean wants(PacketInfo packetInfo) | ||
public synchronized boolean wants(PacketInfo packetInfo) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why does this need to be synchronized now? We're only parallelizing the send out of a single packet at a time, right? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Since the receive pipeline farms out the parallelized There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hm, I think this could result in reordering the packets: I don't think we'd guarantee that the lock on Endpoint A will be taken when processing packet "1" before a thread handling packet "2" for Endpoint A is scheduled. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's true. It's not the end of the world if it happens; worst case the destination sends a spurious NACK. Alternately we could synchronize the sender to wait for all its tasks. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, I'm thinking something more like the latter as maintaining packet order within our own pipeline would be desirable, I think. Depending on how fancy we want/need to get, we could also look at mailboxes per source endpoint for the packets, and we only process one packet from one endpoint at a time (but could still parallelize across packets for multiple endpoints). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unfortunately I discovered that synchronizing to wait for the tasks won't work, because they're executed on the same task pool as the rtp receiver, and an earlier task waiting for a later task is a deadlock. I don't quite follow your description of the fancier methods, but they potentially sound promising if this turns out to be needed. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My thought was that we could do something like:
Now, this functions the same way our other queues do (we process the packets within a single queue serially), we just have N of them. Basically mirror the input queue we have for each endpoint already. This way we can parallelize the work across endpoints, but not within a single endpoint. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, I see. The disadvantage (over what I have now) is that the packet cloning optimization is weaker -- right now as long as at least one of the last batch of packet handlers wants the packet, we get the optimization, whereas with this the very last one would need to. If we do have these queues, I'm not sure there's an architectural difference between them and the RTP sender queues. Is there any way we could just inject these as a leading node (or other process) on the RTP sender rather than having a separate, third set of queues? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Hmm, maybe I'm missing something here. The optimization should be the same, as we're still looking at every potential "sender" for a given ingress packet, so we can know how many. We just don't do anything with the next packet for that same "receiver" until we're done with the previous one. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As long as there's more than one reference outstanding in the If every projection processing runs in parallel, we have to have a reference for every sender, rather than a reference for every batch of senders. |
||
{ | ||
if (!isTransportConnected()) | ||
{ | ||
|
@@ -441,7 +441,7 @@ else if (packet instanceof RtcpFbPliPacket || | |
* TODO Brian | ||
*/ | ||
@Override | ||
public void send(PacketInfo packetInfo) | ||
public synchronized void send(PacketInfo packetInfo) | ||
{ | ||
Packet packet = packetInfo.getPacket(); | ||
if (packet instanceof VideoRtpPacket) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,125 @@ | ||
/* | ||
* Copyright @ 2018 - present 8x8, Inc. | ||
* | ||
* Licensed under the Apache License, Version 2.0 (the "License"); | ||
* you may not use this file except in compliance with the License. | ||
* You may obtain a copy of the License at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* Unless required by applicable law or agreed to in writing, software | ||
* distributed under the License is distributed on an "AS IS" BASIS, | ||
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
* See the License for the specific language governing permissions and | ||
* limitations under the License. | ||
*/ | ||
|
||
|
||
package org.jitsi.videobridge.util; | ||
|
||
import org.jitsi.nlj.*; | ||
|
||
import java.util.function.*; | ||
|
||
public class PacketInfoDistributor | ||
{ | ||
private final PacketInfo packetInfo; | ||
private int totalReferences; | ||
private int outstandingReferences; | ||
|
||
private Consumer<PacketInfo> prevConsumer = null; | ||
|
||
public PacketInfoDistributor(PacketInfo pi, int count) | ||
{ | ||
packetInfo = pi; | ||
totalReferences = count; | ||
outstandingReferences = count; | ||
/* We create a PacketInfoDistributor on each packet so we don't want to instantiate a new logger each time. */ | ||
} | ||
|
||
public void setCount(int count) | ||
{ | ||
if (outstandingReferences < totalReferences) | ||
{ | ||
throw new IllegalStateException("Count set after references have been taken"); | ||
} | ||
totalReferences = count; | ||
outstandingReferences = count; | ||
} | ||
|
||
public synchronized PacketInfo previewPacketInfo() | ||
{ | ||
if (outstandingReferences <= 0) | ||
{ | ||
throw new IllegalStateException("Packet previewed after all references taken"); | ||
} | ||
return packetInfo; | ||
} | ||
|
||
public void usePacketInfoReference(Consumer<PacketInfo> consumer) | ||
{ | ||
// We want to avoid calling 'clone' for the last receiver of this packet | ||
// since it's unnecessary. To do so, we'll wait before we clone and send | ||
// to an interested handler until after we've determined another handler | ||
// is also interested in the packet. We'll give the last handler the | ||
// original packet (without cloning). | ||
Consumer<PacketInfo> c1 = null, c2 = null; | ||
PacketInfo p1 = null, p2 = null; | ||
synchronized(this) { | ||
outstandingReferences--; | ||
if (outstandingReferences < 0) | ||
{ | ||
throw new IllegalStateException("Too many references taken"); | ||
} | ||
if (prevConsumer != null) { | ||
c1 = prevConsumer; | ||
p1 = packetInfo.clone(); | ||
} | ||
prevConsumer = consumer; | ||
if (outstandingReferences == 0) | ||
{ | ||
c2 = prevConsumer; | ||
p2 = packetInfo; | ||
} | ||
} | ||
|
||
if (c1 != null) | ||
{ | ||
c1.accept(p1); | ||
} | ||
if (c2 != null) | ||
{ | ||
c2.accept(p2); | ||
} | ||
} | ||
|
||
public void releasePacketInfoReference() | ||
{ | ||
Consumer<PacketInfo> c = null; | ||
PacketInfo p = null; | ||
synchronized (this) | ||
{ | ||
outstandingReferences--; | ||
if (outstandingReferences < 0) | ||
{ | ||
throw new IllegalStateException("Too many references taken"); | ||
} | ||
if (outstandingReferences == 0) | ||
{ | ||
if (prevConsumer != null) | ||
{ | ||
c = prevConsumer; | ||
p = packetInfo; | ||
} | ||
else | ||
{ | ||
ByteBufferPool.returnBuffer(packetInfo.getPacket().getBuffer()); | ||
} | ||
} | ||
} | ||
if (c != null) | ||
{ | ||
c.accept(p); | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suspect this may depend on the number of cores a machine has. Shall we make it configurable?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Possibly; this was just a quick and dirty check to see how much difference this made.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We already populate the
CPU_POOL
based on the number of cores, so if anything maybe we'd want to base this on the size of the cpu pool, so we only make this decision in one place.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking at YourKit,
AbstractExectorService.submit
seems to spend a fair amount of its time inLinkedBlockingQueue.offer
which has to acquire a lock, so we may well want more tasks than cores here.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, we probably just shouldn't be using
offer
there.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's inside the Java implementation -- and never mind, I was looking at putting things on the executor queue, not the execution of the queues.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, I see what you mean now. 👍