Skip to content
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

Add per-path queues, prioritize control messages over traffic #873

Open
wants to merge 7 commits into
base: dev
from

Conversation

@tewinget
Copy link
Collaborator

tewinget commented Oct 25, 2019

TODO: make sure that paths we create (as a client) are removed from the per-path queues.

TODO: research / trial & error on how best to manage messages sent per tick

Copy link
Collaborator

notlesh left a comment

I have a few comments that are probably worth reviewing, but looking good so far.

What are your thoughts on the performance so far?

#include <linux/if.h>
extern "C" unsigned int
if_nametoindex(const char* __ifname) __THROW;
#endif

This comment has been minimized.

Copy link
@notlesh

notlesh Oct 25, 2019

Collaborator

Is your editor beautifying code here?

This comment has been minimized.

Copy link
@tewinget

tewinget Oct 29, 2019

Author Collaborator

Must be -- I did make format before committing.

This comment has been minimized.

Copy link
@tewinget

tewinget Oct 29, 2019

Author Collaborator

Also this is gone in dev now, so it's no longer relevant after I rebased.


using SendStatusHandler = std::function< void(SendStatus) >;

static const size_t MAX_PATH_QUEUE_SIZE = 40;
static const size_t MAX_OUTBOUND_QUEUE_SIZE = 200;
static const size_t MAX_OUTBOUND_MESSAGES_PER_TICK = 20;

This comment has been minimized.

Copy link
@notlesh

notlesh Oct 25, 2019

Collaborator

constexpr might be appropriate here

This comment has been minimized.

Copy link
@tewinget

tewinget Oct 29, 2019

Author Collaborator

As far as I recall it's a semantic choice in this case but the two are equivalent. Can change if desired though.

entry.router = remote;
entry.pathid = pathid;
Comment on lines +227 to +230

This comment has been minimized.

Copy link
@notlesh

notlesh Oct 25, 2019

Collaborator

Would it be better to accept remote and pathid by value? This allows the caller (or compiler) to move rather than copy when calling the function. Example:

OutboundMessageHandler::QueueOutboundMessage(RouterID remote,
                                               Message &&msg,
                                               PathID_t pathid)
// ...
entry.router = std::move(remote);
entry.pathid = std::move(pathid);

This pattern allows the caller to decide whether a copy is necessary or not, and avoids a copy internally.

This comment has been minimized.

Copy link
@tewinget

tewinget Oct 29, 2019

Author Collaborator

True, but it also requires the caller to know that the value will be moved and act accordingly. Tough call imo. Will change if desired for certain.

This comment has been minimized.

Copy link
@notlesh

notlesh Oct 29, 2019

Collaborator

I think the difference is that by taking a ref, you always require a copy, but by taking a value and doing a move, you sometimes incur one copy.

That said, this call is nothing compared to the network I/O that follows... :)

This comment has been minimized.

Copy link
@tewinget

tewinget Oct 29, 2019

Author Collaborator

I agree that it would be slightly more cpu efficient to do a move, I just don't know if it's worth it to require any part of the code calling this function to be aware of that side-effect. I can do it either way, of course.

if(not removedPaths.empty())
{
removedSomePaths = true;
}
else
{
removedSomePaths = false;
}
Comment on lines 266 to 273

This comment has been minimized.

Copy link
@notlesh

notlesh Oct 25, 2019

Collaborator
Suggested change
}
removedSomePaths = (not removedPaths.empty());

This comment has been minimized.

Copy link
@notlesh

notlesh Oct 25, 2019

Collaborator

Looks like multi-line suggestions aren't a thing (see here), but you get my idea.

This comment has been minimized.

Copy link
@tewinget

tewinget Oct 29, 2019

Author Collaborator

Yeah, that'd definitely better. Not sure why I wrote it so verbosely. Changing.


void
OutboundMessageHandler::SendRoundRobin()
{

This comment has been minimized.

Copy link
@notlesh

notlesh Oct 25, 2019

Collaborator

Do we need a lock on _mutex here?

This comment has been minimized.

Copy link
@tewinget

tewinget Oct 29, 2019

Author Collaborator

This will only be called from the Logic thread, so it should not. I could add a TRACEY lock that's null for release builds if desired?

This comment has been minimized.

Copy link
@notlesh

notlesh Oct 29, 2019

Collaborator

Ah, ok. Other code uses these annotations to mark mutex usage:

https://github.com/abseil/abseil-cpp/blob/master/absl/base/thread_annotations.h

Although I don't see anything specifically to convey this condition.

This ties in to the conversation we had the other day about annotating functions regarding the thread(s) they're supposed to run in, it would probably be more useful to annotate the thread pattern here than the mutex strategy.

PathID_t pathid = std::move(roundRobinOrder.front());
roundRobinOrder.pop();

if(outboundMessageQueues[pathid].size() > 0)

This comment has been minimized.

Copy link
@notlesh

notlesh Oct 25, 2019

Collaborator

This will have the side effect of creating an empty entry in outboundMessageQueues when one doesn't exist (and, in these cases, this conditional will still evaluate to false).

On that note, is there any logic to clean up the queues when they're no longer used (timeout, etc.)?

I see that these entries are erased below, but what about the side effect mentioned here?

This comment has been minimized.

Copy link
@tewinget

tewinget Oct 29, 2019

Author Collaborator

if a pathid is in roundRobinOrder then it has a corresponding outboundMessageQueues entry.


ILinkManager *_linkManager;
std::shared_ptr< Logic > _logic;

const PathID_t zeroID;

This comment has been minimized.

Copy link
@notlesh

notlesh Oct 25, 2019

Collaborator

A comment explaining this magic value might help people who aren't aware of its purpose

This comment has been minimized.

Copy link
@majestrate

majestrate Oct 28, 2019

Collaborator

couldn't this be a static member?

This comment has been minimized.

Copy link
@tewinget

tewinget Oct 29, 2019

Author Collaborator

Will make static and add comment.

This comment has been minimized.

Copy link
@tewinget

tewinget Oct 29, 2019

Author Collaborator

Also, that reminds me that I need to make nodes reject paths with the zero ID, and path builds not use it. Doing that now.

@tewinget tewinget force-pushed the tewinget:path-queues branch from 21d2741 to ef98681 Oct 29, 2019
@notlesh

This comment has been minimized.

Copy link
Collaborator

notlesh commented Oct 29, 2019

Is this still a WIP?

@tewinget

This comment has been minimized.

Copy link
Collaborator Author

tewinget commented Oct 29, 2019

Is this still a WIP?

Apparently so, considering Travis doesn't seem to like it. Taking a look here soon.

@tewinget

This comment has been minimized.

Copy link
Collaborator Author

tewinget commented Oct 29, 2019

I derped and changed OutboundMessageHandler::zeroID to be static but didn't initialize/define it in the cpp file. Fixed now, just double checking before I commit and push.

@tewinget

This comment has been minimized.

Copy link
Collaborator Author

tewinget commented Oct 30, 2019

Ok, I think it's good to go now. Paths build just fine with make testnet but of course it's harder to test whether path traffic is still fine.

@@ -31,8 +31,14 @@ namespace llarp
for(size_t idx = 0; idx < hsz; ++idx)
{
hops[idx].rc = h[idx];
hops[idx].txID.Randomize();
hops[idx].rxID.Randomize();
while(hops[idx].txID.IsZero())

This comment has been minimized.

Copy link
@majestrate

majestrate Oct 30, 2019

Collaborator
do
{
   randomize
}
while(iszero)

This comment has been minimized.

Copy link
@majestrate

majestrate Oct 30, 2019

Collaborator

defensive style to prevent possible use of unzero'd but not random memory

@neuroscr neuroscr referenced this pull request Nov 3, 2019
@tewinget tewinget force-pushed the tewinget:path-queues branch from 7818795 to 23a9773 Nov 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.