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.

Sign up for GitHub

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

Pathfinding dispatch improvements #344

Closed
wants to merge 1 commit into from
Closed

Pathfinding dispatch improvements #344

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
60 changes: 33 additions & 27 deletions src/ripple_app/paths/PathRequest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,8 @@ const boost::shared_ptr<InfoSub>& subscriber, int id, PathRequests& owner,
, wpSubscriber (subscriber)
, jvStatus (Json::objectValue)
, bValid (false)
, iLastIndex (0)
, mLastIndex (0)
, mInProgress (false)
, iLastLevel (0)
, bLastSuccess (false)
, iIdentifier (id)
Expand Down Expand Up @@ -77,38 +78,43 @@ bool PathRequest::isValid ()

bool PathRequest::isNew ()
{
// does this path request still need its first full path
return iLastIndex.load() == 0;
ScopedLockType sl (mIndexLock);

// does this path request still need its first full path
return mLastIndex == 0;
}

bool PathRequest::needsUpdate (bool newOnly, LedgerIndex index)
{
LedgerIndex lastIndex = iLastIndex.load();
for (;;)
ScopedLockType sl (mIndexLock);

if (mInProgress)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor comment - you could just make this one compound if:

if (mInProgress || (newOnly && mLastIndex) || mLastIndex >= index)
return false;

but my major comment is: how can this work!? It seems to me after the first time you go through this routine, mInProgress will be set to true, and then the request will NEVER indicate again that it needs an update!!

{
if (newOnly)
{
// Is this request new
if (lastIndex != 0)
return false;

// This thread marks this request handled
if (iLastIndex.compare_exchange_weak (lastIndex, 1,
std::memory_order_release, std::memory_order_relaxed))
return true;
}
else
{
// Has the request already been handled?
if (lastIndex >= index)
return false;

// This thread marks this request handled
if (iLastIndex.compare_exchange_weak (lastIndex, index,
std::memory_order_release, std::memory_order_relaxed))
return true;
}
// Another thread is handling this
return false;
}

if (newOnly && (mLastIndex != 0))
{
// Only handling new requests, this isn't new
return false;
}

if (mLastIndex >= index)
{
return false;
}

mInProgress = true;
return true;
}

void PathRequest::updateComplete ()
{
ScopedLockType sl (mIndexLock);

assert (mInProgress);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and everything mysteriously dies?

More logging would be really, really useful here. Remember, if you're putting an assertion in, your idea is that one day this will trip, and then you'll need to diagnose what went wrong.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a pretty common pattern. Before an object is worked on, a flag is set to indicate that it is being worked on. When the thing is done working on it, it clears that flag. The name, 'mInProgress' tries to make clear that this pattern is being used. But I guess it wasn't so clear.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it was pretty clear; I'm not sure what the issue is? mInProgress is set to true at some point, and then when updateComplete is called it's set back to false.

mInProgress = false;
}

bool PathRequest::isValid (RippleLineCache::ref crCache)
Expand Down
5 changes: 4 additions & 1 deletion src/ripple_app/paths/PathRequest.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ class PathRequest :
bool isValid ();
bool isNew ();
bool needsUpdate (bool newOnly, LedgerIndex index);
void updateComplete ();
Json::Value getStatus ();

Json::Value doCreate (const boost::shared_ptr<Ledger>&, const RippleLineCache::pointer&,
Expand Down Expand Up @@ -93,7 +94,9 @@ class PathRequest :

bool bValid;

std::atomic<LedgerIndex> iLastIndex;
LockType mIndexLock;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

but... this isn't actually an "index lock" is it? I haven't looked at PathRequest completely but you're introducing this for the first time, so all the uses of it have to be here - and it seems to be locking mInProgress only!

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gah, ignore that - sorry!

LedgerIndex mLastIndex;
bool mInProgress;

int iLastLevel;
bool bLastSuccess;
Expand Down
1 change: 1 addition & 0 deletions src/ripple_app/paths/PathRequests.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,6 +90,7 @@ void PathRequests::updateAll (Ledger::ref inLedger, CancelCallback shouldCancel)
if (!ipSub->getConsumer ().warn ())
{
Json::Value update = pRequest->doUpdate (cache, false);
pRequest->updateComplete ();
update["type"] = "path_find";
ipSub->send (update, false);
remove = false;
Expand Down