Bug 2791 - Interference Helper gives negative trace value for Interference Power
Interference Helper gives negative trace value for Interference Power
Status: RESOLVED FIXED
Product: ns-3
Classification: Unclassified
Component: wifi
ns-3.27
All All
: P3 major
Assigned To: sebastien.deronne
:
: 2845 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2017-10-05 10:09 EDT by Tom Henderson
Modified: 2018-02-23 18:10 EST (History)
2 users (show)

See Also:


Attachments
proposed solution (11.24 KB, patch)
2018-01-19 16:22 EST, sebastien.deronne
Details | Diff
proposed solution (11.44 KB, patch)
2018-01-20 12:03 EST, sebastien.deronne
Details | Diff
Suggested revised patch (13.26 KB, patch)
2018-01-22 15:48 EST, Peter Barnes
Details | Diff
Suggested revised patch v2 (12.86 KB, patch)
2018-01-23 15:39 EST, Peter Barnes
Details | Diff
updated patch (13.82 KB, patch)
2018-02-01 14:10 EST, sebastien.deronne
Details | Diff
latest solution with replacement of vector by multimap (15.45 KB, patch)
2018-02-03 08:04 EST, sebastien.deronne
Details | Diff
final patch (15.94 KB, patch)
2018-02-07 06:18 EST, sebastien.deronne
Details | Diff
Revised patch, keeps the first zero power noise event (17.01 KB, patch)
2018-02-09 20:13 EST, Peter Barnes
Details | Diff
benchmark program (10.95 KB, text/x-c++src)
2018-02-21 14:42 EST, Tom Henderson
Details

Note You need to log in before you can comment on or make changes to this bug.
Description Tom Henderson 2017-10-05 10:09:42 EDT
Reported on the ns-3-users list; needs to be looked into.
https://groups.google.com/forum/#!topic/ns-3-users/abB_fM7C4rs
Comment 1 sebastien.deronne 2017-10-29 16:25:28 EDT
Can I get a script to reproduce the issue?
Comment 2 sebastien.deronne 2018-01-10 16:18:35 EST
Update from Peter Barnes:

The Wifi InterferenceHelper, as well as the Spectrum model SpectrumInterference class, are vulnerable to roundoff, resulting in negative computed interference (Ix), and hence negative SINR, which triggers an assert.

There have been two recent reports on the users' list:
https://groups.google.com/forum/#!topic/ns-3-users/abB_fM7C4rs
https://groups.google.com/forum/#!topic/ns-3-users/t6L35V_eZ8M
It's not clear either user has a repeatable example :( 

The second thread traces it back to wifi/model/interference-helper.cc:243 InterferenceHelper::CalculateSnr().

It looks like Ix sources are being tracked in a list of deltas, stored as NiChange events.  Look at 220:AppendEvent() for example:

- it adds (or modifies, lines 230-231) a new NiChange:
235:  NiChange (event->GetStartTime (), event->GetRxPowerW (), event)

- it adds a decrement:
237:  NiChange (event->GetEndTime (), -event->GetRxPowerW (), event)

The callers of CalculateSnr() walk the list of NiChange, adding in the delta power stored in each NiChange, to compute the total Ix power.  

When the relevant span includes both ends (start and end) of an Ix event, the design of storing (startTime, +power) then (endTime, -power) leads to adding and subtracting the same value.  In between these addition and subtraction the absolute value of Ix being calculated can have changed significantly larger, leading to roundoff errors in the subtraction, ultimately resulting in negative values.

A quick look suggests SpectrumInterference has the same approach.

To prove that this is the problem a couple of things might help:

1.  Instrument to record the high water mark as the Ix is being computed. This should be significantly larger than the final (negative) Ix value, which lends credence to loss of precision hypothesis.

2.  Check the end result Ix value, if it's negative set it to m_firstPower (it's initial value).  This should be a simple two line patch which either user could test for us.

#2 is just a hack, not the real fix.  We shouldn't settle for this hack a. because it could mask other more serious issues in the future, and b. because there's a better fix.

The real fix is to refactor NiChange from (time, delta) to (time, power).  To add a new Ix record, 

1.  add two NiChange events for start time and and end time of the new Ix source.

2.  For the end time NiChange, set the power to the power of the previous entry (in time).  

3.  For all events in the interval [start, end) (including the new start event, but not the new end event) set power to the power of the entry before the new event.

Step 3 has the effect of adding the new Ix power to the old value, starting at the start time.  Step 2 has the effect of restoring the power to what it would have been, in the absence of the new Ix source, at the end time.  Notice that this algorithm *does not* require any subtraction, only addition.
Comment 3 sebastien.deronne 2018-01-10 16:19:30 EST
*** Bug 2845 has been marked as a duplicate of this bug. ***
Comment 4 sebastien.deronne 2018-01-10 16:28:56 EST
Peter, thanks for the information, I like the idea to avoid subtractions.
In any case, a reproduction script is required to test the solution.
Comment 5 Peter Barnes 2018-01-10 21:22:03 EST
Sorry for duplicate post.  Cranky hotel network is partly to blame.  Since the same code is used in wifi and spectrum,
Comment 6 Peter Barnes 2018-01-10 21:24:22 EST
Since the same code is used in wifi and spectrum (and bugzilla can't tag two components in the same bug report) I DID intend to create 2847 as a placeholder against spectrum.
Comment 7 Tom Henderson 2018-01-10 22:41:44 EST
This patch can reproduce the condition:

diff -r 71da905a6eb9 src/wifi/model/interference-helper.cc
--- a/src/wifi/model/interference-helper.cc    Wed Jan 10 19:29:50 2018 +0100
+++ b/src/wifi/model/interference-helper.cc    Wed Jan 10 15:31:45 2018 -0800
@@ -287,6 +287,7 @@
     }
   ni->insert (ni->begin (), NiChange (event->GetStartTime (), noiseInterference, event));
   ni->push_back (NiChange (event->GetEndTime (), 0, event));
+  NS_ABORT_MSG_IF (noiseInterference < 0, "Noise interference < 0 :" << noiseInterference);
   return noiseInterference;
 }

./waf --run 'power-adaptation-interference --simuTime=5'

aborted. cond="noiseInterference < 0", msg="Noise interference < 0 :-2.47387e-27", file=../src/wifi/model/interference-helper.cc, line=290
terminate called without an active exception 


It seems that it might be benign in most of our use cases, but in the LAA code, if it arises, there is a later attempt to take the log of it which causes a nan which then causes downstream issues.
Comment 8 sebastien.deronne 2018-01-11 01:37:41 EST
Peter, Tom,
Since we are 3 people looking at it right now, who is going to work on a patch?
I have some time this weekend to work on a solution based on Peter's proposal.
Comment 9 Peter Barnes 2018-01-11 23:47:43 EST
@Tom:  I don't think it's benign.  Just try to calculate a SINR then use an error model...

@Sebastien:  I won't be able to work on this, so charge ahead.

The only diagnostic bit I can add is, following up on Tom's reproducible example, I added some logging to trace the evolution of m_firstPower:

+1.000922702s 1 InterferenceHelper:AppendEvent(): [DEBUG] firstPower: 0, delta: 1.346294318435293e-10, time: +1000662702.0ns
+1.000922702s 1 InterferenceHelper:AppendEvent(): [DEBUG] firstPower: 1.346294318435293e-10, delta: 1.346294318435293e-13, time: +1000662703.0ns
+1.000922702s 1 InterferenceHelper:AppendEvent(): [DEBUG] firstPower: 1.347640612753728e-10, delta: -1.346294318435293e-10, time: +1000774702.0ns
+1.000922702s 1 InterferenceHelper:AppendEvent(): [DEBUG] firstPower: 1.346294318435268e-13, delta: -1.346294318435293e-13, time: +1000906703.0ns
aborted. cond="m_firstPower < 0", msg="+1.000922702sfirstPower: -2.473867798773093e-27", file=../../src/wifi/model/interference-helper.cc, line=231

Simplified, the evolution is

m_firstPower   delta
0              1.346294e-10
1.346294e-10   1.346294e-13
1.347640e-10  -1.346294e-10
1.346294e-13  -1.346294e-13

Notice that we:  add A, add B, sub A, sub B, where B = A/1000.  We lose 3 digits of precision from B in the addition when it's normalized to A's exponent, but not on the subtraction; we are not subtracting the same number as added, and in this case we end up negative.

(Side note:  curious that the digits in A and B are exactly the same, only the exponent is different.  I haven't tried to understand this part at all, but seems weird.)
Comment 10 Peter Barnes 2018-01-11 23:51:49 EST
Rereading my suggestion, I see I left off one important piece:  folding in the new power:

3.  For all events in the interval [start, end) (including the new start event, but not the new end event) set power to 

    the power of the entry before the new event
 +  the new Ix power
Comment 11 sebastien.deronne 2018-01-12 02:46:14 EST
Peter, thanks for your very interesting feedback that will for sure help in fixing this issue.
I will try to work on a patch this weekend.
Comment 12 Tom Henderson 2018-01-12 10:19:40 EST
(In reply to Peter Barnes from comment #9)
> @Tom:  I don't think it's benign.  Just try to calculate a SINR then use an
> error model...

Sorry, poor word choice-- I meant that we are not seeing the effect in our existing use cases in our tests and examples (i.e. it is not triggering any faults which is why it has been hidden for a while)-- not that it isn't possibly causing some harm.

I agree with trying your refactoring.
Comment 13 sebastien.deronne 2018-01-14 04:41:28 EST
I am facing one question: how should we handle the case where multiple events exists for a same time position? This occurs if two packets arrive at the same moment.
Comment 14 sebastien.deronne 2018-01-14 16:26:58 EST
I should post a solution this week.
Comment 15 Peter Barnes 2018-01-16 11:51:14 EST
(In reply to sebastien.deronne from comment #13)
> I am facing one question: how should we handle the case where multiple
> events exists for a same time position? This occurs if two packets arrive at
> the same moment.

It doesn't matter to the algorithm, since the Ix events arrive (are inserted) one after the other.  In other words, assume when about to insert the first event A you have

   Ix0  Ix1  ..  Ixn  Ix(n+1)

where Ix0 stands for the aggregate interference power beginning at the time of event Ix0.  Assume A starts between Ix0 and Ix1, and ends between Ixn and Ix(n+1): 

   Ix0  A  Ix1'  ..  Ixn'  Ix(n+1)

where 'ed values are equal to the un-primed value + the additional power of A,
and A = Ix power at Ix0 + new Ix power from A.

Now we need to insert Ix event B, which ties with A.  It doesn't matter which one lands first:

    Ix0  A  B'  Ix1''  .. ,  where B' = Ix power at A + new Ix power from B
                                   Ix1'' = Ix1' + new Ix power from B

    Ix0  B  A'  Ix1''  .. ,  where B = Ix power at Ix0 + new Ix power from B
                                   A' = Ix power previously at A + new Ix power from B
                                   Ix1'' = Ix1' + new Ix power from B

Notice that in either case Ix1'' = Ix1 + A + B.

For extra credit one could contemplate merging tied events, but there's something in the code which searches for specific events (I don't recall why) which would probably struggle with merged events.
Comment 16 sebastien.deronne 2018-01-18 15:56:07 EST
I do not think we can merge, I had that in mind but I saw a lot of issues there. I am finalizing the patch right now.
Comment 17 sebastien.deronne 2018-01-19 16:22:37 EST
Created attachment 3006 [details]
proposed solution

I took some time this week to implement a solution based on Peter Barnes's proposal.
No regression test is impacted.
Comment 18 Peter Barnes 2018-01-19 17:55:40 EST
It would be nice if AddNiChangeEvent() returned the iterator of the new event, then the following loop at line 240 would be a lot simpler:

auto first = AddNiChangeEvent (NiChange (event->GetStartTime (), previousPowerStart + event->GetRxPowerW (), event));
auto last = AddNiChangeEvent (NiChange (event->GetEndTime (), previousPowerEnd, event));
for (auto i = first; i < last; ++i)
{  
  i->AddPower (event->GetRxPowerW ());
}

Otherwise +1

I love working with the ns-3 team!
Comment 19 sebastien.deronne 2018-01-20 12:03:21 EST
Created attachment 3007 [details]
proposed solution

Peter, I like a lot your idea, but I am facing an issue when I do:
auto last = AddNiChangeEvent (...)
At that moment, a new vector is reallocated (since its size is incremented), so the distance between last and first has no sense anymore.
A solution is to return the distance to the first element instead of an iterator.
Please find the updated patch in attachment.
Comment 20 Peter Barnes 2018-01-22 15:48:51 EST
Created attachment 3008 [details]
Suggested revised patch

I've got some picky questions about the patch.  I'll post my version (untested) for comparison/comment.

First, the original choice of  typedef std::vector <NiChange> NiChanges; is problematic, for a container which is supposed to be sorted.  std::multimap would be much better.  Should we make that change now? Several loops would disappear, as well as the GetNextPosition, GetPreviousPosition...

interference-helper.cc:222:  I don't understand the conditional.  If NiChanges is empty (on inserting the first event), GetPreviousEvent will return a begin which can't be dereferenced.  I think this should stanza should be handling the empty-list case like this:

  double previousPowerStart = 0;  // or should this be m_firstPower?
  auto itPreviousEvent = GetPreviousPosition (event->GetStartTime ());
  if ( m_niChanges.size() > 0 )    {
      // itPrevious points to a real event with start time <= event
      previousPowerStart = itPreviousEvent->GetPower ();
    }

Similar comment on the next stanza, with similar alteration.

interference-helper.cc:234:  I had to stare to see that the new power wasn't being added twice.  I'd prefer:

  auto first = AddNiChangeEvent (NiChange (event->GetStartTime (), previousPowerStart, event));
  auto last = AddNiChangeEvent (NiChange (event->GetEndTime (), previousPowerEnd, event));
  for (auto i = first; i < last; i++)
    {
      m_niChanges.at (i).AddPower (event->GetRxPowerW ());
    }

interference-helper.cc:280:  this loop exits on the first iteration
  for (NiChanges::const_iterator i = eventIterator + 1; i != m_niChanges.end (); ++i)


interference-helper.cc:364 and 
      noiseInterferenceW = j->GetPower () - powerW;
The NiChange j has the total received power, so we subtract the signal power to get the noise?


interference-helper.cc:870:  simplify using GetPreviousPosition:
  m_rxing = false;
  //Update m_firstPower for frame capture
  auto it = GetPreviousPosition (Simulator::Now ());
  m_firstPower = it->GetPower ();
Comment 21 sebastien.deronne 2018-01-22 15:53:56 EST
Peter, I see you still have GetDelta calls in your patch, is it a mistake?

Also, this cannot work:
  auto first = AddNiChangeEvent (NiChange (event->GetStartTime (), previousPowerStart, event));
  auto last = AddNiChangeEvent (NiChange (event->GetEndTime (), previousPowerEnd, event));
  for (auto i = first; i < last; i++)
    {
      m_niChanges.at (i).AddPower (event->GetRxPowerW ());
    }
Because when we insert the event, it is reallocated so we need to use the distance instead as in my latest patch.
Comment 22 Peter Barnes 2018-01-22 17:13:50 EST
Whoops, looks like I applied to an old version.  I can't seem to ssh to/from code.nsnam.org ATM, probably our firewall.

first and last are just indices into the vector, not pointer-style iterators, so first isn't invalidated by inserting last into the vector.  If you don't like this, all the more reason to switch to multi_map.
Comment 23 sebastien.deronne 2018-01-23 15:22:06 EST
Multimap seems indeed appealing here :-)
Can you please provide your updated patch? Normally, you should be able to pull from ns-3-dev.
Comment 24 Peter Barnes 2018-01-23 15:39:22 EST
Created attachment 3009 [details]
Suggested revised patch v2

Try this.  Still not based on current ns-3-dev, so the line numbers are probably wrong, but no more references to delta.

Uncompiled or tested.
Comment 25 sebastien.deronne 2018-01-31 16:05:14 EST
Peter, I have plenty of issues with the new patch, I fixed compilation issues but tests are failing or crashing. I think it might be easier if you point the difference between our patches so that I can continue.
Comment 26 sebastien.deronne 2018-01-31 17:00:35 EST
(In reply to sebastien.deronne from comment #25)
> Peter, I have plenty of issues with the new patch, I fixed compilation
> issues but tests are failing or crashing. I think it might be easier if you
> point the difference between our patches so that I can continue.

I started to compare with a diff, I already found some issues at both sides. I will continue to merge and then start to replace with multimap.
Comment 27 sebastien.deronne 2018-02-01 14:10:10 EST
Created attachment 3025 [details]
updated patch

I merged our contributions and fixed issues.
I will now look at adding multimap.
Comment 28 sebastien.deronne 2018-02-01 15:45:10 EST
Peter, do you see something like this?
typedef std::multimap<Time, NiChange> NiChanges;
Where time is removed from the NiChange class.

I indeed see less issues with iterator when using this multimap.
But can we really get a direct function in multimap to replace the GetPrevious and GetNext functions as you mentioned?
Comment 29 Peter Barnes 2018-02-01 16:36:59 EST
> But can we really get a direct function in multimap to replace 
> the GetPrevious and GetNext functions as you mentioned?

Won't --it work?

InterferenceHelper::NiChanges::iterator
InterferenceHelper::GetPreviousPosition (Time moment)
{
  auto it = m_niChanges.find (moment);
  // Have to handle the case where moment is the first NiChange,
  // so find returns begin()
  return --it;
}
Comment 30 sebastien.deronne 2018-02-03 08:04:45 EST
Created attachment 3026 [details]
latest solution with replacement of vector by multimap

Peter, is this fitting what you have in mind? Please let me know so that we can come up to a conclusion this week :-)
Comment 31 Peter Barnes 2018-02-05 17:52:52 EST
This generally looks good to me.  I assume it passes all tests?

A few nitpicks:

interference-helper.cc:245:  could this be a find(), instead of a loop?  
  auto it = m_niChanges.find (event->GetStartTime ());
  for ( ; it != m_niChanges.end (); ++it)
      if (it->second.GetEvent () == event) break;
  noiseInterference = it->second.GetPower ();

:296 and :324:  I'd prefer to have ++j at the top of the loop
:362 and :740:  ditto

:798 and :804: What happens if the list is empty?  Won't GetNext return end() == begin(), then --it gives you an error?  at least when you dereference.

:808:  GetPositionBeforeTime() seems redundant with GetPreviousPosition().  It looks like there are three positions being used:
  GetPositionBeforeTime()  event before a set of tied events, 
  GetPreviousPosition()    the last tied event
  GetNextPosition()        the first event after a set of ties
Could we use just two of these?  Can we insert at the beginning of a tie set, instead of the end (to eliminate GetPreviousPosition)?

interference-helper.h:153:  Not obvious why this can't still be a const function.  Comment it?
:263:                       ditto
Comment 32 sebastien.deronne 2018-02-07 06:18:40 EST
Created attachment 3030 [details]
final patch

Peter, I addressed your remaining comments, I think it is now ready to get pushed.
Comment 33 Peter Barnes 2018-02-07 14:32:43 EST
interference-helper.h:323-329:  remove obsolete GetNextPosition doxy and declaration

interference-helper.cc:247:  shouldn't this be outside the loop?

interference-helper.cc:798:  GetPreviousPosition()
I don't understand this implementation.  The basic implementation is obviously

  auto next = GetNextPosition (moment); 
  auto prev = --next;  // explicit variable for clarity of discussion
  return prev;

as you have.  This is complicated by the edge cases:

  begin != end  (NiChanges has entries)
    next != begin:  simple case:  prev will be valid.  This includes next == end.
    next == begin:  prev won't be valid.  Not clear what to do in this case.  

  begin == end (list is empty)
    next == begin == end:  prev won't be valid.  Not clear what to do in this case.  

One resolution to the invalid cases could be:  
  Insert a new zero power event prior to begin, but with no associated Event.  At what time?  The moment query argument?

Looking at where GetPreviousPosition is used, perhaps the lack of Event isn't an issue in practice:  

line 186 and following:  references only NiChange.m_power
line 209:  references only NiChange.m_power
Comment 34 sebastien.deronne 2018-02-07 14:59:04 EST
(In reply to Peter Barnes from comment #33)
> interference-helper.h:323-329:  remove obsolete GetNextPosition doxy and
> declaration

Oups, thanks :-)

> 
> interference-helper.cc:247:  shouldn't this be outside the loop?

Correct

> 
> interference-helper.cc:798:  GetPreviousPosition()
> I don't understand this implementation.  The basic implementation is
> obviously
> 
>   auto next = GetNextPosition (moment); 
>   auto prev = --next;  // explicit variable for clarity of discussion
>   return prev;
> 
> as you have.  This is complicated by the edge cases:
> 
>   begin != end  (NiChanges has entries)
>     next != begin:  simple case:  prev will be valid.  This includes next ==
> end.
>     next == begin:  prev won't be valid.  Not clear what to do in this case.
> 
> 
>   begin == end (list is empty)
>     next == begin == end:  prev won't be valid.  Not clear what to do in
> this case.  
> 
> One resolution to the invalid cases could be:  
>   Insert a new zero power event prior to begin, but with no associated
> Event.  At what time?  The moment query argument?
> 
> Looking at where GetPreviousPosition is used, perhaps the lack of Event
> isn't an issue in practice:  

I rolled back to the previous implementation.
We actually cannot encounter issues because we do some checks at other places to dereference only if size if not 0.

> 
> line 186 and following:  references only NiChange.m_power
> line 209:  references only NiChange.m_power

Not sure I understand what you mean.
Comment 35 Peter Barnes 2018-02-07 15:19:46 EST
>> One resolution to the invalid cases could be:  
>>   Insert a new zero power event prior to begin, but with no associated
>> Event.  At what time?  The moment query argument?
>> 
>> Looking at where GetPreviousPosition is used, perhaps the lack of Event
>> isn't an issue in practice:  
> 
> I rolled back to the previous implementation.
> We actually cannot encounter issues because we do some checks at other 
> places to dereference only if size if not 0.
> 
>> line 186 and following:  references only NiChange.m_power
>> line 209:  references only NiChange.m_power
> 
> Not sure I understand what you mean.

Only that inserting a zero power NiChange, with no value for Event, would be ok since no callers actually reference the previous event, only previous power.
Comment 36 Peter Barnes 2018-02-07 15:29:06 EST
> I rolled back to the previous implementation.
> We actually cannot encounter issues because we do some checks at other 
> places to dereference only if size if not 0.

Um, I don't think so:

183	InterferenceHelper::GetEnergyDuration (double energyW) const
184	{
185	  Time now = Simulator::Now ();
186	  auto i = GetPreviousPosition (now);
187	  Time end = i->first;

If NiChanges is empty, GetPrev returns --begin, which is invalid.  This is a public function, so it can be called at arbitrary times, even when the list is empty.  (For example, an upper layer could use this (or a similar function) to query the noise level, in order to decide if it's low enough to bother transmitting.)

Remember this patch isn't just for Wifi, but also Spectrum, so it's worth getting a general solution here.

Give a thought to adding an NiChange as suggested; it would be clean and cover all the cases.
Comment 37 sebastien.deronne 2018-02-07 16:09:01 EST
(In reply to Peter Barnes from comment #36)
> > I rolled back to the previous implementation.
> > We actually cannot encounter issues because we do some checks at other 
> > places to dereference only if size if not 0.
> 
> Um, I don't think so:
> 
> 183	InterferenceHelper::GetEnergyDuration (double energyW) const
> 184	{
> 185	  Time now = Simulator::Now ();
> 186	  auto i = GetPreviousPosition (now);
> 187	  Time end = i->first;
> 
> If NiChanges is empty, GetPrev returns --begin, which is invalid.  This is a
> public function, so it can be called at arbitrary times, even when the list
> is empty.  (For example, an upper layer could use this (or a similar
> function) to query the noise level, in order to decide if it's low enough to
> bother transmitting.)
> 
> Remember this patch isn't just for Wifi, but also Spectrum, so it's worth
> getting a general solution here.
> 
> Give a thought to adding an NiChange as suggested; it would be clean and
> cover all the cases.

You're correct :-)
But I am not in favor of adding a dummy NiChange, I think this would be cleaner to have a check in GetEnergyDuration:
if empty or if next is equal to begin, duration returned should simply be 0.
The other places using GetPrevious are safe since they include a check on the size.
Comment 38 sebastien.deronne 2018-02-07 16:25:50 EST
> > interference-helper.cc:247:  shouldn't this be outside the loop?
> 
> Correct
> 

I am correcting myself. Actually this was on purpose, otherwise we have to make this more complex by doing --it -> read -> ++it.
Comment 39 Peter Barnes 2018-02-07 16:32:23 EST
> I am correcting myself. Actually this was on purpose, otherwise we have to 
> make this more complex by doing --it -> read -> ++it.

Ok.  The suggestion in Comment 32 seems more natural to me...
Comment 40 Peter Barnes 2018-02-07 16:39:23 EST
> But I am not in favor of adding a dummy NiChange, I think this would be 
> cleaner 

Adding a dummy NiChange means the function always does what it says, which is pretty clean, rather than have to do fix-ups elsewhere in the cases when GetPrev doesn't do what it says.

> to have a check in GetEnergyDuration:
> if empty or if next is equal to begin, duration returned should simply be 
> 0.  The other places using GetPrevious are safe since they include a check 
> on the size.

That other place would also be simpler, just

  double previousPowerStart = GetPreviousPosition (event->GetStartTime ())->second.GetPower ();
  double previousPowerEnd = GetPreviousPosition (event->GetEndTime ())->second.GetPower ();

I'll leave it up to you, just my $0.02
Comment 41 sebastien.deronne 2018-02-08 04:20:03 EST
(In reply to Peter Barnes from comment #40)
> > But I am not in favor of adding a dummy NiChange, I think this would be 
> > cleaner 
> 
> Adding a dummy NiChange means the function always does what it says, which
> is pretty clean, rather than have to do fix-ups elsewhere in the cases when
> GetPrev doesn't do what it says.
> 
> > to have a check in GetEnergyDuration:
> > if empty or if next is equal to begin, duration returned should simply be 
> > 0.  The other places using GetPrevious are safe since they include a check 
> > on the size.
> 
> That other place would also be simpler, just
> 
>   double previousPowerStart = GetPreviousPosition (event->GetStartTime
> ())->second.GetPower ();
>   double previousPowerEnd = GetPreviousPosition (event->GetEndTime
> ())->second.GetPower ();
> 
> I'll leave it up to you, just my $0.02

Peter, I agree the function should do what it tells.
If it would be possible to add the dummy NiChange just at the begin of the function GetPrevious and not in other part of the code, then it could work I think and it will be the cleanest solution. I'll give a try on that.
Comment 42 sebastien.deronne 2018-02-08 14:39:37 EST
(In reply to sebastien.deronne from comment #41)
> (In reply to Peter Barnes from comment #40)
> > > But I am not in favor of adding a dummy NiChange, I think this would be 
> > > cleaner 
> > 
> > Adding a dummy NiChange means the function always does what it says, which
> > is pretty clean, rather than have to do fix-ups elsewhere in the cases when
> > GetPrev doesn't do what it says.
> > 
> > > to have a check in GetEnergyDuration:
> > > if empty or if next is equal to begin, duration returned should simply be 
> > > 0.  The other places using GetPrevious are safe since they include a check 
> > > on the size.
> > 
> > That other place would also be simpler, just
> > 
> >   double previousPowerStart = GetPreviousPosition (event->GetStartTime
> > ())->second.GetPower ();
> >   double previousPowerEnd = GetPreviousPosition (event->GetEndTime
> > ())->second.GetPower ();
> > 
> > I'll leave it up to you, just my $0.02
> 
> Peter, I agree the function should do what it tells.
> If it would be possible to add the dummy NiChange just at the begin of the
> function GetPrevious and not in other part of the code, then it could work I
> think and it will be the cleanest solution. I'll give a try on that.

Not that obvious because I want to keep the constness...
Peter, can you provide me the lines of code corresponding to your solution?
I unfortunately miss time and need to focus on higher priority items before the upcoming release.
Comment 43 Peter Barnes 2018-02-09 20:13:18 EST
Created attachment 3035 [details]
Revised patch, keeps the first zero power noise event

This patch includes Tom's assert.  Tom's example passes.
Comment 44 sebastien.deronne 2018-02-10 06:42:26 EST
(In reply to Peter Barnes from comment #43)
> Created attachment 3035 [details]
> Revised patch, keeps the first zero power noise event
> 
> This patch includes Tom's assert.  Tom's example passes.

Peter, thanks a lot for having taken this up :-)
I had removed the assert from Tom because we will never again have negative values, so this check is not needed in my opinion.
Comment 45 Tom Henderson 2018-02-14 15:15:06 EST
I wrote a quick benchmark based on test-interference-helper to force feed lots of packets at small interarrival times.  On a static optimized build, with input of 2000 frames per second (50us interarrival), and disabling the WifiPhyStateHelper fatal error that prevent new packet from transmitting while in state TX, I'm seeing about 4% slower runtime with this new patch.  I didn't profile or try to optimize, and if you think it is reasonable performance, feel free to commit.  If you are expecting better performance than this, let me know and we can probably work out a better benchmarking program.
Comment 46 Peter Barnes 2018-02-14 16:56:03 EST
The old method adds two events to the list, but defers computing total Ix power until needed.  This version maintains in the list the total Ix power in every interval, so it's doing more work.  4% on a micro-benchmark doesn't seem too bad.

A quick look with a profiler to see there isn't any easy fixes would be nice, but not necessary IMO.

@Tom:  can you make your benchmark a testcase?
Comment 47 Tom Henderson 2018-02-21 14:41:47 EST
(In reply to Peter Barnes from comment #46)
> The old method adds two events to the list, but defers computing total Ix
> power until needed.  This version maintains in the list the total Ix power
> in every interval, so it's doing more work.  4% on a micro-benchmark doesn't
> seem too bad.
> 
> A quick look with a profiler to see there isn't any easy fixes would be
> nice, but not necessary IMO.
> 
> @Tom:  can you make your benchmark a testcase?

Posted as an example for scratch (not as a testcase).  We could consider to put something like this in as a performance test at some point.
Comment 48 Tom Henderson 2018-02-21 14:42:26 EST
Created attachment 3057 [details]
benchmark program
Comment 49 sebastien.deronne 2018-02-23 17:19:29 EST
Pushed in changeset 13311:a40bedf7801a
Comment 50 Peter Barnes 2018-02-23 18:10:30 EST
What do we have to do to apply this to Spectrum?