Task #3205
closed
Reduce usage of shared_ptr in forwarding
100%
Description
In forwarding pipelines and strategy API, change shared_ptr parameters to constant or mutable reference parameters, wherever it's feasible.
Updated by Junxiao Shi over 8 years ago
- Related to Task #3164: Reduce usage of shared_ptr in FIB/PIT/StrategyChoice/Measurements added
Updated by Junxiao Shi almost 8 years ago
- Related to Feature #3664: Incoming Nack pipeline: consider Link during FIB lookup added
Updated by Junxiao Shi almost 8 years ago
- Status changed from New to In Progress
- Assignee set to Junxiao Shi
- % Done changed from 0 to 20
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.
Updated by Junxiao Shi almost 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.
Updated by Davide Pesavento almost 8 years ago
Junxiao Shi wrote:
FaceTablewill continue to useshared_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.
Updated by Junxiao Shi almost 8 years ago
- Related to Feature #3679: Decouple Forwarder and FaceTable added
Updated by Junxiao Shi over 7 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.
Updated by Junxiao Shi over 7 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".
Updated by Junxiao Shi over 7 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.
Updated by Junxiao Shi over 7 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 retainweak_ptr<pit::Entry>for timersshared_ptr<Face>is used inFaceTable, which is justified in note-4shared_ptr<subclass-of-Tag>is used inClientControlStrategy; this API is under control of ndn-cxx
Updated by Junxiao Shi over 7 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.
Updated by Davide Pesavento over 7 years ago
Junxiao Shi wrote:
There has been a suggestion that
Strategytriggers can be wrapped, so that if a strategy implementation mistakenly retains ashared_ptr<pit::Entry>, it would hit an assertion error.
However, wrapping triggers are insufficient, because a strategy could retain aweak_ptr<pit::Entry>from a trigger, and then lock it into ashared_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?)
Updated by Junxiao Shi over 7 years ago
- Status changed from Code review to Closed
Updated by Davide Pesavento over 7 years ago
The NFD Developer's Guide still refers to setStrategyInfo, that doesn't exist anymore since commit fc021864608d9235c6baa4712fb15221cd2af842.
Updated by Junxiao Shi over 7 years ago
nfd-docs:commit:befb69df08c91954fcfbb5d742d0b2d782f41f79 corrects documentation according to note-14.