Task #3205: Reduce usage of shared_ptr in forwarding - NFD - NDN project issue tracking system

https://gerrit.named-data.net/2920 deletes fibEntry parameter in strategy APIs.

Strategy can call const fib::Entry& Strategy::lookupFib(const pit::Entry&) to lookup FIB when necessary.

The benefits are:

Strategy can always access the latest effective FIB entry.
Strategy doesn't need to retain a FIB entry reference, saving a bit of memory.
If a strategy trigger doesn't use the FIB entry, ContentStore miss pipeline or incoming Nack pipeline doesn't need to perform FIB lookup, eliminating an obvious waste.

NFD devguide will be updated at the end.

Actions

Copy link

Updated by Junxiao Shi over 8 years ago

% Done changed from 20 to 40

https://gerrit.named-data.net/2949 changes FaceTable::get return type to Face*.

https://gerrit.named-data.net/2950 changes FaceTable::const_iterator to dereference to Face&.

https://gerrit.named-data.net/2951 changes FaceTable::afterAdd and FaceTable::beforeRemove signals to pass Face& as parameter.

FaceTable will continue to use shared_ptr<Face> because face ownership is also retained by the channel or protocol factory that creates the face.

I don't intend to change this ownership model.

Actions

Copy link

Updated by Davide Pesavento over 8 years ago

Junxiao Shi wrote:

FaceTable will continue to use shared_ptr<Face> because face ownership is also retained by the channel or protocol factory that creates the face.

I don't intend to change this ownership model.

Makes sense.

Actions

Copy link

Updated by Junxiao Shi over 8 years ago

Related to Feature #3679: Decouple Forwarder and FaceTable added

Actions

Copy link

Updated by Junxiao Shi over 8 years ago

Performance comparison for shared_ptr reduction commits so far:

before: ndn-cxx:commit:6b872a93ba573fbf696e3a5d19d757de2362cd63 nfd:commit:b260017fec141f0834f14a914b042318cadca786 https://travis-ci.org/yoursunny/ndn-cxx-breaks/jobs/149811087

after: ndn-cxx:commit:b3015bdfb7afda4caca5b6caac214e48aeb4baf5 nfd:commit:38f4ce9f256ecee6ede456998083ffbbbcf1684c https://travis-ci.org/yoursunny/ndn-cxx-breaks/jobs/149784238

benchmark	before	after
CS find(miss)-insert 400000	22417ms	16838ms
CS insert-find(hit) 400000	23503ms	17381ms
CS find(leftmost) 200000	1301ms	227ms
CS find(rightmost) 200000	1374ms	321ms
PIT+FIB simple exchanges	9846ms	10185ms

I'm unsure why CS benchmarks are improving, given none of the changes touch the CS.

There isn't significant difference in PIT+FIB benchmark results, although I have improved FIB a little bit.

Actions

Copy link

Updated by Junxiao Shi over 8 years ago

% Done changed from 40 to 50

https://gerrit.named-data.net/3066 changes shared_ptr<pit::Entry> (pass-by-value) parameters on strategy triggers to const shared_ptr<pit::Entry>&.

As quoted in #1912-6, const shared_ptr<>& is suitable when it's unsure whether shared ownership would be retained.

In the case of strategy triggers, although strategies must not retain shared_ptr<pit::Entry> after the trigger function returns, they are allowed to keep weak_ptr<pit::Entry> for timers or similar purpose.
However, only a small subset of strategy triggers keep weak_ptr<pit::Entry>, so this fits the guideline of "unsure whether shared ownership would be retained".

Actions

Copy link

Updated by Junxiao Shi over 8 years ago

% Done changed from 50 to 70

https://gerrit.named-data.net/3072 changes shared_ptr<pit::Entry> parameters in Forwarder pipelines and Strategy actions to const shared_ptr<pit::Entry>&.
Helper functions in Forwarder accepts either const shared_ptr<pit::Entry>& if it would call into strategy, or pit::Entry& if it would not call into strategy.

https://gerrit.named-data.net/3073 avoids storing shared_ptr<pit::Entry> in StrategyTester.

Actions

Copy link

#10

Updated by Junxiao Shi over 8 years ago

Status changed from In Progress to Code review
% Done changed from 70 to 100

https://gerrit.named-data.net/3093 avoids shared_ptr in StrategyInfoHost.

The remaining usages of shared_ptr in forwarding are:

shared_ptr<pit::Entry> is used in pipelines, because strategy may need to retain weak_ptr<pit::Entry> for timers
shared_ptr<Face> is used in FaceTable, which is justified in note-4
shared_ptr<subclass-of-Tag> is used in ClientControlStrategy; this API is under control of ndn-cxx

Actions

Copy link

#11

Updated by Junxiao Shi over 8 years ago

There has been a suggestion that Strategy triggers can be wrapped, so that if a strategy implementation mistakenly retains a shared_ptr<pit::Entry>, it would hit an assertion error.
However, wrapping triggers are insufficient, because a strategy could retain a weak_ptr<pit::Entry> from a trigger, and then lock it into a shared_ptr<pit::Entry> and retain that in a timer.
Thus, I decide not to make that change.

Actions

Copy link

#12

Updated by Davide Pesavento over 8 years ago

Junxiao Shi wrote:

There has been a suggestion that Strategy triggers can be wrapped, so that if a strategy implementation mistakenly retains a shared_ptr<pit::Entry>, it would hit an assertion error.
However, wrapping triggers are insufficient, because a strategy could retain a weak_ptr<pit::Entry> from a trigger, and then lock it into a shared_ptr<pit::Entry> and retain that in a timer.
Thus, I decide not to make that change.

Yes, of course. As long as we're giving strategies direct access to a weak_ptr, there will always be a way to circumvent any checks we might add. Moreover, this "wrapping" requires some amount of refactoring... so it's probably best to spend our time designing and implementing a "PIT entry handle" class that really limits what strategies can do with the entry. (btw do we have a redmine ticket for this task?)

Actions

Copy link

#13