On Tue, Sep 10, 2013 at 10:16:07PM +0400, Vladislav Sterzhanov wrote: > Thank you for comments, toad! Just a bit of my motivation for each point: > > >> We do not send anything until we can send a full packet. > > Even if the packet is already overdue? Yeah, I understand the motivation > for coalescing, but the previous code did send the packet even if there's > no fullPacketQueued() in case it found any which deadline is already past > now. There was a separate if-branch for it in the algo. The current > coalesce-until-we-can seems more reasonable in many cases. Am I missing > something or it was also a bug?
Yes, we will send a less-than-full packet if we can. However we don't send
anything (except an ack-only packet) if we can't send a full packet:
canSendThrottled is only true if we can send a full packet. The reasons for
this relate to fairness as well as efficiency: If we do what your patch does
then when we have limited bandwidth we'll constantly send under-sized packets
to peers with small queues, because we wake up and we have enough bandwidth to
send a small packet but not enough to send a big packet, and then this happens
again, and again, and again, each time nibbling away at the bandwidth that we
need for the peers with big messages queued, which get starved. Hence the
fairly absolute rule: We do not send ANYTHING, except for ack-only packets,
unless there is enough bandwidth available to send a full packet.
>
>> IMHO what we need to do here is always go back around the loop after
>> sending a packet. Your code eventually had a separate loop which re-queries
>> each node; easier to use the real logic IMHO. We can't just check the peer
>> we sent on because others might have it, as you discovered; and going back
>> over all the peers is IMHO ugly, easier to use the same code we use for
>> selecting a peer to send to in the first place, fewer special cases.
>
> Yes, general code re-use is an awesome thing, but in this concrete case it
> completely ruins the transparency of logic - those checks will be
> distributed across the whole realRun(), which is already kinda huge.
Which checks? All you have to do is:
if(peer.maybeSendPacket(...)) {
... loads of stuff ...
}
with the much simpler:
if(peer.maybeSendPacket(...)) {
nextActionTime = now
}
And preserve the existing code to update nextActionTime, most of which we need
anyway?
> Understand all the picture of when and by what concrete decision in the
> algorithm the actuall change to nextActionTime in the given situation is
> applied is increasingly hard. That's one of the main reasons why it took so
> much time to examine it (of course, except for the fact that this bug could
> reside in any of the classes up-the-hierarchy). Moreover, even if not count
> the bugs already mentioned, the previous approach led to the other one
> (fixing which will mix up the logic even further) - just a simple example
> to be clear: imagine that it has sent the packet with the deadline 10ms
> from now to the node A. The following packet in it's queue has the deadline
> of 20ms. The queue of the node B consists of only one packet with deadline
> 15ms. So, since it only checks the node's A packetqueue once again after
> the packet is sent it will either have to wake up in 10ms (deadline of the
> sent packet) with no reason to actually do so, either wait() 20ms and skip
> the B's deadline. So, in any case, either it should query all the nodes
> once again (which in turn is much more transparent and compact), either
> track the second best result (which is less expandable and more obscure).
Ummm, that's what I said! You have to query all the nodes again - by going back
around the loop, i.e. by setting nextActionTime = now.
signature.asc
Description: Digital signature
_______________________________________________ Devl mailing list Devl@freenetproject.org https://emu.freenetproject.org/cgi-bin/mailman/listinfo/devl
