Project

General

Profile

Actions

Feature #3177

closed

LpFace counters

Added by Junxiao Shi over 8 years ago. Updated over 8 years ago.

Status:
Closed
Priority:
Normal
Assignee:
Category:
Faces
Target version:
Start date:
Due date:
% Done:

100%

Estimated time:
3.00 h

Description

Develop FaceCounters for use with LinkService+Transport face structure.


Related issues 5 (1 open4 closed)

Related to ndn-cxx - Feature #3174: NACK countersClosedEric Newberry

Actions
Blocked by NFD - Task #3088: Refactor Face as LinkService+TransportClosedEric Newberry

Actions
Blocks NFD - Task #3172: Refactor Face: completionClosedJunxiao Shi

Actions
Blocked by NFD - Task #1729: Add byte counters to FaceCountersClosedJunxiao Shi

Actions
Blocks NFD - Task #3325: Improve FaceDataset and GeneralStatusDataset testsIn Progress

Actions
Actions #1

Updated by Junxiao Shi over 8 years ago

  • Blocked by Task #3088: Refactor Face as LinkService+Transport added
Actions #2

Updated by Junxiao Shi over 8 years ago

  • Blocks Task #3172: Refactor Face: completion added
Actions #3

Updated by Junxiao Shi over 8 years ago

Actions #4

Updated by Junxiao Shi over 8 years ago

The difficulty we met during initial LpFace implementation was:

  • Each of Transport and LinkService provides some counters.
  • Face (aka LpFace) should collect counters from both Transport and LinkService.
  • The FaceCounters class inherits from both LinkLayerCounters and NetworkLayerCounters, so the counters from Transport and LinkServices would need to be copied to FaceCounters.
  • But a counter is noncopyable.

I have two solutions.

SOLUTION 1

  • Transport and LinkService each has its own set of counters.
  • FaceCounters is a composition of Transport's counters and LinkService's counters. It contains a pointer to each set of counters.
  • benefit: An individual Transport/LinkService can potentially provide additional counters than the standard ones. They can be accessed in simulations, etc.

SOLUTION 2

  • All counters are in the Face.
  • Transport and LinkService can increment a counter through .getFace accessor
  • drawback: Transport/LinkService depends on Face for counters. Among others, this complicates unit testing.
  • drawback: Transport/LinkService cannot extend other counters.
Actions #5

Updated by Eric Newberry over 8 years ago

I like the first approach more than the second approach. However, why do we need counters in the Face, in addition to the Transport and LinkService?

Actions #6

Updated by Davide Pesavento over 8 years ago

I'm against solution 2.

Solution 1 sounds acceptable, but I'm wondering why we need this "composition" of counters in the face. If needed, the face can simply be a façade for LinkService+Transport counters.

Actions #7

Updated by Alex Afanasyev over 8 years ago

I'm against solution 2 as well. Solution 1 could be acceptable for now, but in the long-term we should design a flexible counters: when we publish counters as part of status dataset, different combination of transport/link service would potentially give different set of counters.

Actions #8

Updated by Spyros Mastorakis over 8 years ago

I do not like solution 2 as well.

Solution 1 seems better, however, I have the same question with Davide:

Why do we need this "composition" of counters in the face?

Actions #9

Updated by Junxiao Shi over 8 years ago

The idea of having counters in Face is to hide the LinkService+Transport structure from management.

Management should be able to pull counters (and also properties) from Face in order to populate datasets, without knowing the internal structure of Face.

Actions #10

Updated by Davide Pesavento over 8 years ago

Exactly, so what's the problem with having one or more façade methods in LpFace for the LinkService+Transport counters? We already have a similar thing for the scope, persistency, link type, etc...

Actions #11

Updated by Junxiao Shi over 8 years ago

Answer to note-10:

If Face class have .getLinkServiceCounters() .getTransportCounters(), it's still exposing the LinkService+Transport internal structure to management.

Face class should a single .getCounters().
The return value of this function may have getters like .getNInInterests() .getNInBytes() that call the corresponding getters in TransportCounters LinkServiceCounters.

Actions #12

Updated by Davide Pesavento over 8 years ago

Junxiao Shi wrote:

Face class should a single .getCounters().
The return value of this function may have getters like .getNInInterests() .getNInBytes() that call the corresponding getters in TransportCounters LinkServiceCounters.

Yes, this is what I meant. The wrapper returned from getCounters() should be as thin as possible though.

Actions #13

Updated by Junxiao Shi over 8 years ago

  • Blocked by Task #1729: Add byte counters to FaceCounters added
Actions #14

Updated by Junxiao Shi over 8 years ago

The semantics of byte counters was defined in #1729 as:

Byte count field reflect number of bytes received or sent on link layer. Those counters include link layer headers imposed by NFD (NDNLP or LocalControlHeader), but exclude headers of underlying protocol (Ethernet or TCP or UDP).

To fulfill this semantics, we could increment the byte counters at Transport::send and Transport::receive.

In the design, there's also a "transport packet counter" that should be incremented at the same time.

The concern is: should the byte counters be incremented when the transport is DOWN?

For RECEIVE counters, there's little difference.

For SEND counters:

  • Transport::send can still be used to attempt sending a packet even if the transport is DOWN.
  • For UDP or Ethernet transport, the packet is still sent through the socket, and still consumes bandwidth.
  • For TCP transport, the packet is definitely dropped when TCP connection is not established, and thus does not consume bandwidth.

If we would like to differentiate, the counter should not be incremented in Transport::send.

Transport could expose a method that subclass should invoke after a packet is sent onto the network, which increments the counter.

Actions #15

Updated by Davide Pesavento over 8 years ago

Junxiao Shi wrote:

  • For UDP or Ethernet transport, the packet is still sent through the socket, and still consumes bandwidth.

does it? I guess it depends on how you define bandwidth... but if I unplug my ethernet cable I'm definitely not consuming bandwidth...

If we would like to differentiate, the counter should not be incremented in Transport::send.

Transport could expose a method that subclass should invoke after a packet is sent onto the network, which increments the counter.

I don't know... I'd rather keep a consistent behavior across all transports.

Actions #16

Updated by Junxiao Shi over 8 years ago

  • Assignee set to Junxiao Shi
  • Estimated time changed from 2.00 h to 3.00 h

I'd rather keep a consistent behavior across all transports.

Agreed. I'll increment counters in Transport::send and Transport::receive, only when transport is UP.

Actions #17

Updated by Junxiao Shi over 8 years ago

  • Status changed from New to In Progress
Actions #18

Updated by Junxiao Shi over 8 years ago

  • % Done changed from 0 to 20

http://gerrit.named-data.net/2549

I'm adopting note-4 SOLUTION 1.

In patchset1, I'm showing how "an individual Transport/LinkService can provide additional counters than the standard ones".

I need a review on this inheritance-based design before continuing onto more changes.

Actions #19

Updated by Junxiao Shi over 8 years ago

One issue with patchset1 is:

  • GenericLinkService::Counters inherits from LinkService::Counters
  • LinkService::m_counters has type LinkService::Counters and is returned by LinkService::getCounters() virtual method
  • GenericLinkService::m_counters has type GenericLinkService::Counters and is returned by GenericLinkService::getCounters() virtual method which overrides LinkService::getCounters()
  • In GenericLinkService instance, the memory for LinkService::m_counters is wasted because it's never used

An alternate is to make LinkService::getCounters() pure virtual, but this forces every LinkService subclass to declare this method, which I want to avoid.

(it's not a problem for LinkService because we only have two in the plan; but Transport::Counters needs the same inheritance capability and there are many transports)

@Davide suggested "diamond inheritance with virtual bases" as a solution but I don't understand.
Can you explain more?

Actions #20

Updated by Davide Pesavento over 8 years ago

See patch set #2

Actions #21

Updated by Junxiao Shi over 8 years ago

I disagree with the sample in patchset2 nfd:commit:d1c96431b9f567cccf6ba1a3b5d16350d6b9e1c1

Its basic idea is:

struct LsCounters
{
  counters for LinkService
};

class LinkService : public LsCounters
{
};

struct GlsCounters : public virtual LsCounters
{
  extra counters for GenericLinkService
};

class GenericLinkService : public LinkService, public virtual GlsCounters
{
};

The problem is: inheritance represents an is-a relation, but a LinkService is not a Counters but has-a Counters.

Note: changing into non-public inheritance does not solve this problem.

Actions #22

Updated by Davide Pesavento over 8 years ago

Junxiao Shi wrote:

The problem is: inheritance represents an is-a relation, but a LinkService is not a Counters but has-a Counters.

Note: changing into non-public inheritance does not solve this problem.

Why do you say that it doesn't solve the "problem"? Only public inheritance establishes an is-a relationship, protected inheritance does not, because it would violate the substitution principle. So effectively the fact that LsCounters is a protected base of LinkService is an implementation detail. Also consider the fact that private and protected inheritance are usually considered much more similar to composition than to type inheritance.

(obviously using protected inheritance requires moving getCounters back into LinkService, but this shouldn't affect the rest of the design)

Actions #23

Updated by Junxiao Shi over 8 years ago

Answer to note-22:

It seems that I misunderstood the semantics of inheritance.
http://www.cprogramming.com/tutorial/private.html says:

Private inheritance does not model an is-a relationship in the same way that public inheritance does. Instead, private inheritance refers to the idea of being "implemented in terms of a".

Also, CodeStyle does not forbid non-public inheritance.
Thus, I'll adopt patchset2 design but use protected inheritance.

Actions #24

Updated by Davide Pesavento over 8 years ago

One drawback of virtual inheritance is that you have to use dynamic_cast in place of static_cast...

Actions #25

Updated by Junxiao Shi over 8 years ago

http://gerrit.named-data.net/2549 patchset4 fulfills note-23 plan.

I'll soon continue onto Transport counters.

Actions #26

Updated by Junxiao Shi over 8 years ago

  • % Done changed from 20 to 50

http://gerrit.named-data.net/2549 patchset7 adds Transport::Counters according to note-16 semantics.

The remaining work is to figure out what to do with LpFace::getCounters and LpFaceWrapper::getCounters.

Actions #27

Updated by Junxiao Shi over 8 years ago

In patchset7, GenericLinkService has two drop counters:

  • nInParseError: incoming packets dropped before reassembly because it cannot be parsed as LpPacket; e.g. top-level TLV-TYPE is not LpPacket or Interest or Data, FragIndex is greater than FragCount
  • nInNetInvalid: a reassembled network layer packet is dropped because an error in network layer; e.g. Interest does not have Name field, Data arrives with Nack field

This distinction is necessary, because they have different granularity: each nInParseError increment drops one LpPacket, while each nInNetInvalid increment can drop multiple LpPackets (all fragments that form this network layer packet).

Actions #28

Updated by Davide Pesavento over 8 years ago

Junxiao Shi wrote:

The remaining work is to figure out what to do with LpFace::getCounters and LpFaceWrapper::getCounters.

Can you elaborate on what the problem is?

Actions #29

Updated by Junxiao Shi over 8 years ago

  • Status changed from In Progress to Code review
  • % Done changed from 50 to 100
Actions #30

Updated by Junxiao Shi over 8 years ago

This feature has been split to two commits:

http://gerrit.named-data.net/2549 adds LinkService and Transport counters;

http://gerrit.named-data.net/2563 uses LinkService and Transport counters to compose FaceCounters, replacing old FaceCounters.

Actions #31

Updated by Junxiao Shi over 8 years ago

  • Blocks Task #3325: Improve FaceDataset and GeneralStatusDataset tests added
Actions #32

Updated by Junxiao Shi over 8 years ago

  • Status changed from Code review to Closed
Actions

Also available in: Atom PDF