Code review - Issue 14632043: Introduce additional time unitshttps://codereview.appspot.com/2013-12-06T18:43:59+00:00rietveld
Message from unknown
2013-10-13T00:40:22+00:00Vedran Miletićurn:md5:50b19db1703e463725c4f44116a2838b
Message from pdbj@mac.com
2013-10-18T18:25:56+00:00Peter Barnesurn:md5:dd9c85d7e432762f758aed478abeaa36
Hello Vedran,
When you tossed this into the bug954 discussion I thought it would be easy to add later, and I had it on my to do list RSN.
However, I think there are some subtleties with the way we do conversions using integer arithmetic. See my comments on time.cc lines 201 and 235 for a start.
https://codereview.appspot.com/14632043/diff/1/src/core/model/nstime.h
File src/core/model/nstime.h (right):
https://codereview.appspot.com/14632043/diff/1/src/core/model/nstime.h#newcode111
src/core/model/nstime.h:111: A = 0, //!< year
Please document that you've chosen 365.25 days per year, a Julian year.
https://codereview.appspot.com/14632043/diff/1/src/core/model/nstime.h#newcode216
src/core/model/nstime.h:216: * - `a` (years)
Should we support alternative symbols when converting from strings? 'mn', 'hr', 'day', 'week', 'y', 'yr', 'year'
https://codereview.appspot.com/14632043/diff/1/src/core/model/time.cc
File src/core/model/time.cc (right):
https://codereview.appspot.com/14632043/diff/1/src/core/model/time.cc#newcode201
src/core/model/time.cc:201: const int32_t coefficient [LAST] = { 315576, 6048, 864, 36, 6, 1, 1, 1, 1, 1, 1 };
I think the choice of int32_t here (and int for quotient, below) is a mistake. It leads to premature truncation in computing factor. To see the problem, run
Time::SetResolution (Time::D);
std::cout << (Days (4 * 365.25).GetYears ()) << std::endl;
I get '4.0027397', but it should be 4.
I don't have a good suggestion yet on how to fix this...
https://codereview.appspot.com/14632043/diff/1/src/core/model/time.cc#newcode219
src/core/model/time.cc:219: double realFactor = std::pow (10, shift)
I need 'std::pow (10, (double)shift)' to compile. std::pow(int, int) is ambiguous)
https://codereview.appspot.com/14632043/diff/1/src/core/model/time.cc#newcode225
src/core/model/time.cc:225: // to avoid checking equality of doubles
You could avoid this completely by
if (realFactor > 1) { ... }
else if (realFactor < 1) { ... }
else { ... }
https://codereview.appspot.com/14632043/diff/1/src/core/model/time.cc#newcode235
src/core/model/time.cc:235: info->timeFrom = int64x64_t (factor);
We should rethink the use of int64x64_t here. Because we treat factor > 1 and factor < 1 separately, only one of the 64-bit fields is ever being used. We could just as well store factor in info, and rely on to/fromMul to select multiply or divide in the conversions.
Message from rivanvx@gmail.com
2013-10-21T07:51:17+00:00Vedran Miletićurn:md5:9dd7f0d57901c1dd3538b9390abcfd29
On 2013/10/18 18:25:56, Peter Barnes wrote:
> Hello Vedran,
>
> When you tossed this into the bug954 discussion I thought it would be easy to
> add later, and I had it on my to do list RSN.
>
> However, I think there are some subtleties with the way we do conversions using
> integer arithmetic. See my comments on time.cc lines 201 and 235 for a start.
Hi Peter,
thanks for your review. My comments are inline.
> https://codereview.appspot.com/14632043/diff/1/src/core/model/nstime.h
> File src/core/model/nstime.h (right):
>
> https://codereview.appspot.com/14632043/diff/1/src/core/model/nstime.h#newcode111
> src/core/model/nstime.h:111: A = 0, //!< year
> Please document that you've chosen 365.25 days per year, a Julian year.
Sure. By the way, do you believe that 365 days would have been a better choice?
> https://codereview.appspot.com/14632043/diff/1/src/core/model/nstime.h#newcode216
> src/core/model/nstime.h:216: * - `a` (years)
> Should we support alternative symbols when converting from strings? 'mn', 'hr',
> 'day', 'week', 'y', 'yr', 'year'
Sure, why not?
> https://codereview.appspot.com/14632043/diff/1/src/core/model/time.cc
> File src/core/model/time.cc (right):
>
> https://codereview.appspot.com/14632043/diff/1/src/core/model/time.cc#newcode201
> src/core/model/time.cc:201: const int32_t coefficient [LAST] = { 315576, 6048,
> 864, 36, 6, 1, 1, 1, 1, 1, 1 };
> I think the choice of int32_t here (and int for quotient, below) is a mistake.
> It leads to premature truncation in computing factor. To see the problem, run
>
> Time::SetResolution (Time::D);
> std::cout << (Days (4 * 365.25).GetYears ()) << std::endl;
>
> I get '4.0027397', but it should be 4.
>
> I don't have a good suggestion yet on how to fix this...
Damn, I have to nail this.
> https://codereview.appspot.com/14632043/diff/1/src/core/model/time.cc#newcode219
> src/core/model/time.cc:219: double realFactor = std::pow (10, shift)
> I need 'std::pow (10, (double)shift)' to compile. std::pow(int, int) is
> ambiguous)
Fixed.
> https://codereview.appspot.com/14632043/diff/1/src/core/model/time.cc#newcode225
> src/core/model/time.cc:225: // to avoid checking equality of doubles
> You could avoid this completely by
>
> if (realFactor > 1) { ... }
> else if (realFactor < 1) { ... }
> else { ... }
Can you guarantee that our 'intuitive' realFactor == 1 will not end in realFactor > 1 or realFactor < 1 branch unless we handle it first?
> https://codereview.appspot.com/14632043/diff/1/src/core/model/time.cc#newcode235
> src/core/model/time.cc:235: info->timeFrom = int64x64_t (factor);
> We should rethink the use of int64x64_t here. Because we treat factor > 1 and
> factor < 1 separately, only one of the 64-bit fields is ever being used. We
> could just as well store factor in info, and rely on to/fromMul to select
> multiply or divide in the conversions.
Neat. Let me try to nail the imprecision first, and then we can discuss this further.
Message from rivanvx@gmail.com
2013-10-21T09:03:22+00:00Vedran Miletićurn:md5:b0c5b74b49efacb2611b7c495844a2a5
On 2013/10/21 07:51:17, Vedran Miletić wrote:
> On 2013/10/18 18:25:56, Peter Barnes wrote:
> > Hello Vedran,
> >
> > When you tossed this into the bug954 discussion I thought it would be easy to
> > add later, and I had it on my to do list RSN.
> >
> > However, I think there are some subtleties with the way we do conversions
> using
> > integer arithmetic. See my comments on time.cc lines 201 and 235 for a start.
>
> Hi Peter,
>
> thanks for your review. My comments are inline.
>
> > https://codereview.appspot.com/14632043/diff/1/src/core/model/nstime.h
> > File src/core/model/nstime.h (right):
> >
> >
> https://codereview.appspot.com/14632043/diff/1/src/core/model/nstime.h#newcode111
> > src/core/model/nstime.h:111: A = 0, //!< year
> > Please document that you've chosen 365.25 days per year, a Julian year.
>
> Sure. By the way, do you believe that 365 days would have been a better choice?
>
> >
> https://codereview.appspot.com/14632043/diff/1/src/core/model/nstime.h#newcode216
> > src/core/model/nstime.h:216: * - `a` (years)
> > Should we support alternative symbols when converting from strings? 'mn',
> 'hr',
> > 'day', 'week', 'y', 'yr', 'year'
>
> Sure, why not?
>
> > https://codereview.appspot.com/14632043/diff/1/src/core/model/time.cc
> > File src/core/model/time.cc (right):
> >
> >
> https://codereview.appspot.com/14632043/diff/1/src/core/model/time.cc#newcode201
> > src/core/model/time.cc:201: const int32_t coefficient [LAST] = { 315576, 6048,
> > 864, 36, 6, 1, 1, 1, 1, 1, 1 };
> > I think the choice of int32_t here (and int for quotient, below) is a mistake.
>
> > It leads to premature truncation in computing factor. To see the problem, run
> >
> > Time::SetResolution (Time::D);
> > std::cout << (Days (4 * 365.25).GetYears ()) << std::endl;
> >
> > I get '4.0027397', but it should be 4.
> >
> > I don't have a good suggestion yet on how to fix this...
>
> Damn, I have to nail this.
Can't reproduce actually (this doesn't change anything however). For me (Debian testing amd64, GCC 4.8.1), this results in:
assert failed. cond="quotient * coefficient[(int) unit] == coefficient[i]", file=../src/core/model/time.cc, line=209
terminate called without an active exception
But I _will_ dig deeper and test on some machines running Debian stable amd64.
Regards,
V.
Message from rivanvx@gmail.com
2013-10-21T13:23:06+00:00Vedran Miletićurn:md5:cfd93a188f6e53262f94939683fb4621
On 2013/10/21 09:03:22, Vedran Miletić wrote:
> On 2013/10/21 07:51:17, Vedran Miletić wrote:
> > On 2013/10/18 18:25:56, Peter Barnes wrote:
> > > Hello Vedran,
> > >
> > > When you tossed this into the bug954 discussion I thought it would be easy
> to
> > > add later, and I had it on my to do list RSN.
> > >
> > > However, I think there are some subtleties with the way we do conversions
> > using
> > > integer arithmetic. See my comments on time.cc lines 201 and 235 for a
> start.
> >
> > Hi Peter,
> >
> > thanks for your review. My comments are inline.
> >
> > > https://codereview.appspot.com/14632043/diff/1/src/core/model/nstime.h
> > > File src/core/model/nstime.h (right):
> > >
> > >
> >
> https://codereview.appspot.com/14632043/diff/1/src/core/model/nstime.h#newcode111
> > > src/core/model/nstime.h:111: A = 0, //!< year
> > > Please document that you've chosen 365.25 days per year, a Julian year.
> >
> > Sure. By the way, do you believe that 365 days would have been a better
> choice?
> >
> > >
> >
> https://codereview.appspot.com/14632043/diff/1/src/core/model/nstime.h#newcode216
> > > src/core/model/nstime.h:216: * - `a` (years)
> > > Should we support alternative symbols when converting from strings? 'mn',
> > 'hr',
> > > 'day', 'week', 'y', 'yr', 'year'
> >
> > Sure, why not?
> >
> > > https://codereview.appspot.com/14632043/diff/1/src/core/model/time.cc
> > > File src/core/model/time.cc (right):
> > >
> > >
> >
> https://codereview.appspot.com/14632043/diff/1/src/core/model/time.cc#newcode201
> > > src/core/model/time.cc:201: const int32_t coefficient [LAST] = { 315576,
> 6048,
> > > 864, 36, 6, 1, 1, 1, 1, 1, 1 };
> > > I think the choice of int32_t here (and int for quotient, below) is a
> mistake.
> >
> > > It leads to premature truncation in computing factor. To see the problem,
> run
> > >
> > > Time::SetResolution (Time::D);
> > > std::cout << (Days (4 * 365.25).GetYears ()) << std::endl;
> > >
> > > I get '4.0027397', but it should be 4.
> > >
> > > I don't have a good suggestion yet on how to fix this...
> >
> > Damn, I have to nail this.
>
> Can't reproduce actually (this doesn't change anything however). For me (Debian
> testing amd64, GCC 4.8.1), this results in:
>
> assert failed. cond="quotient * coefficient[(int) unit] == coefficient[i]",
> file=../src/core/model/time.cc, line=209
> terminate called without an active exception
>
> But I _will_ dig deeper and test on some machines running Debian stable amd64.
>
> Regards,
> V.
I understand the issue. What we could do is drop Week (I highly doubt it would be used much if at all anyway), and put Year to be 365 days. Then we would have
const int32_t coefficient [LAST] = { 315360, 864, 36, 6, 1, 1, 1, 1, 1, 1 };
Now each unit is an integer multiple of a lower unit, and problem is solved.
Comments, opinions?
Regards,
Vedran
Message from tomh.org@gmail.com
2013-11-22T17:17:50+00:00Tom Hendersonurn:md5:0e9f7dbd3210511d41b7b70f7f57fee0
On 2013/10/21 13:23:06, Vedran Miletić wrote:
> On 2013/10/21 09:03:22, Vedran Miletić wrote:
> > On 2013/10/21 07:51:17, Vedran Miletić wrote:
> > > On 2013/10/18 18:25:56, Peter Barnes wrote:
> > > > Hello Vedran,
> > > >
> > > > When you tossed this into the bug954 discussion I thought it would be easy
> > to
> > > > add later, and I had it on my to do list RSN.
> > > >
> > > > However, I think there are some subtleties with the way we do conversions
> > > using
> > > > integer arithmetic. See my comments on time.cc lines 201 and 235 for a
> > start.
> > >
> > > Hi Peter,
> > >
> > > thanks for your review. My comments are inline.
> > >
> > > > https://codereview.appspot.com/14632043/diff/1/src/core/model/nstime.h
> > > > File src/core/model/nstime.h (right):
> > > >
> > > >
> > >
> >
> https://codereview.appspot.com/14632043/diff/1/src/core/model/nstime.h#newcode111
> > > > src/core/model/nstime.h:111: A = 0, //!< year
> > > > Please document that you've chosen 365.25 days per year, a Julian year.
> > >
> > > Sure. By the way, do you believe that 365 days would have been a better
> > choice?
> > >
> > > >
> > >
> >
> https://codereview.appspot.com/14632043/diff/1/src/core/model/nstime.h#newcode216
> > > > src/core/model/nstime.h:216: * - `a` (years)
> > > > Should we support alternative symbols when converting from strings? 'mn',
> > > 'hr',
> > > > 'day', 'week', 'y', 'yr', 'year'
> > >
> > > Sure, why not?
> > >
> > > > https://codereview.appspot.com/14632043/diff/1/src/core/model/time.cc
> > > > File src/core/model/time.cc (right):
> > > >
> > > >
> > >
> >
> https://codereview.appspot.com/14632043/diff/1/src/core/model/time.cc#newcode201
> > > > src/core/model/time.cc:201: const int32_t coefficient [LAST] = { 315576,
> > 6048,
> > > > 864, 36, 6, 1, 1, 1, 1, 1, 1 };
> > > > I think the choice of int32_t here (and int for quotient, below) is a
> > mistake.
> > >
> > > > It leads to premature truncation in computing factor. To see the problem,
> > run
> > > >
> > > > Time::SetResolution (Time::D);
> > > > std::cout << (Days (4 * 365.25).GetYears ()) << std::endl;
> > > >
> > > > I get '4.0027397', but it should be 4.
> > > >
> > > > I don't have a good suggestion yet on how to fix this...
> > >
> > > Damn, I have to nail this.
> >
> > Can't reproduce actually (this doesn't change anything however). For me
> (Debian
> > testing amd64, GCC 4.8.1), this results in:
> >
> > assert failed. cond="quotient * coefficient[(int) unit] == coefficient[i]",
> > file=../src/core/model/time.cc, line=209
> > terminate called without an active exception
> >
> > But I _will_ dig deeper and test on some machines running Debian stable amd64.
> >
> > Regards,
> > V.
>
> I understand the issue. What we could do is drop Week (I highly doubt it would
> be used much if at all anyway), and put Year to be 365 days. Then we would have
>
> const int32_t coefficient [LAST] = { 315360, 864, 36, 6, 1, 1, 1, 1, 1, 1 };
>
> Now each unit is an integer multiple of a lower unit, and problem is solved.
>
> Comments, opinions?
>
> Regards,
> Vedran
I support this solution.
Please add ns-3-reviews@googlegroups.com to the cc list.
Any other comments, or should we proceed with this for ns-3.19?
Message from unknown
2013-11-24T13:11:46+00:00Vedran Miletićurn:md5:137cae6bfe091ef8a0fc4ca0b2517f2b
Message from rivanvx@gmail.com
2013-11-24T13:20:36+00:00Vedran Miletićurn:md5:84d1b6433cb763d89e0d757c0cb52fca
Updated, rebased on ns-3-dev (no changes needed though). Year is now 365 days, and week is gone. In-line doxygen documentation has been updated to provide information about year being 365 days, days being 24 hours etc.
However, we have four failing tests (-regression ones with pcap traces). I did some inspection into devices-mesh-dot11s-regression, and it turns out that src/mesh/test/dot11s/hwmp-simplest-regression-test-0-1.pcap and generated pacp differ "only" in:
< 17 1.942149 00:00:00_00:00:04 -> Broadcast LLC 76 S, func=RR, N(R)=0; DSAP NULL LSAP Individual, SSAP ISO Network Layer (unofficial?) Command
< 18 1.942192 00:00:00_00:00:04 -> Broadcast LLC 76 S, func=RR, N(R)=0; DSAP NULL LSAP Individual, SSAP 0x1e Response
< 19 1.942399 00:00:00_00:00:03 -> Broadcast 802.11 69 Action, SN=3, FN=0, Flags=o......., SSID=% [truncated]
< 20 1.942657 00:00:00_00:00:04 -> 00:00:00_00:00:03 802.11 63 Action, SN=3, FN=0, Flags=o......., SSID= [truncated]
< 21 1.942673 -> 00:00:00_00:00:04 (RA) 802.11 14 Acknowledgement, Flags=o.......
< 22 1.942805 00:00:00_00:00:03 -> 00:00:00_00:00:04 LLC 76 I, N(R)=0, N(S)=0; DSAP NULL LSAP Individual, SSAP ISO Network Layer (unofficial?) Command
< 23 1.942993 -> 00:00:00_00:00:03 (RA) 802.11 14 Acknowledgement, Flags=o.......
< 24 1.943296 00:00:00_00:00:04 -> 00:00:00_00:00:03 LLC 176 I, N(R)=0, N(S)=0; DSAP NULL LSAP Individual, SSAP ISO Network Layer (unofficial?) Command
< 25 1.943312 -> 00:00:00_00:00:04 (RA) 802.11 14 Acknowledgement, Flags=o.......
< 26 1.952296 00:00:00_00:00:03 -> Broadcast LLC 76 S, func=RR, N(R)=0; DSAP NULL LSAP Individual, SSAP ISO Network Layer (unofficial?) Command
< 27 1.952595 00:00:00_00:00:04 -> 00:00:00_00:00:03 LLC 76 I, N(R)=0, N(S)=0; DSAP NULL LSAP Individual, SSAP ISO Network Layer (unofficial?) Command
< 28 1.952611 -> 00:00:00_00:00:04 (RA) 802.11 14 Acknowledgement, Flags=o.......
< 29 1.952815 00:00:00_00:00:03 -> 00:00:00_00:00:04 LLC 176 I, N(R)=0, N(S)=0; DSAP NULL LSAP Individual, SSAP ISO Network Layer (unofficial?) Command
< 30 1.953135 -> 00:00:00_00:00:03 (RA) 802.11 14 Acknowledgement, Flags=o.......
< 31 1.953324 00:00:00_00:00:03 -> Broadcast LLC 76 S, func=RR, N(R)=0; DSAP NULL LSAP Individual, SSAP 0x1e Response
---
> 17 1.942369 00:00:00_00:00:04 -> Broadcast LLC 76 S, func=RR, N(R)=0; DSAP NULL LSAP Individual, SSAP ISO Network Layer (unofficial?) Command
> 18 1.942412 00:00:00_00:00:04 -> Broadcast LLC 76 S, func=RR, N(R)=0; DSAP NULL LSAP Individual, SSAP 0x1e Response
> 19 1.942619 00:00:00_00:00:03 -> Broadcast 802.11 69 Action, SN=3, FN=0, Flags=o......., SSID=% [truncated]
> 20 1.942877 00:00:00_00:00:04 -> 00:00:00_00:00:03 802.11 63 Action, SN=3, FN=0, Flags=o......., SSID= [truncated]
> 21 1.942893 -> 00:00:00_00:00:04 (RA) 802.11 14 Acknowledgement, Flags=o.......
> 22 1.943025 00:00:00_00:00:03 -> 00:00:00_00:00:04 LLC 76 I, N(R)=0, N(S)=0; DSAP NULL LSAP Individual, SSAP ISO Network Layer (unofficial?) Command
> 23 1.943213 -> 00:00:00_00:00:03 (RA) 802.11 14 Acknowledgement, Flags=o.......
> 24 1.943516 00:00:00_00:00:04 -> 00:00:00_00:00:03 LLC 176 I, N(R)=0, N(S)=0; DSAP NULL LSAP Individual, SSAP ISO Network Layer (unofficial?) Command
> 25 1.943532 -> 00:00:00_00:00:04 (RA) 802.11 14 Acknowledgement, Flags=o.......
> 26 1.953441 00:00:00_00:00:03 -> Broadcast LLC 76 S, func=RR, N(R)=0; DSAP NULL LSAP Individual, SSAP ISO Network Layer (unofficial?) Command
> 27 1.953740 00:00:00_00:00:04 -> 00:00:00_00:00:03 LLC 76 I, N(R)=0, N(S)=0; DSAP NULL LSAP Individual, SSAP ISO Network Layer (unofficial?) Command
> 28 1.953756 -> 00:00:00_00:00:04 (RA) 802.11 14 Acknowledgement, Flags=o.......
> 29 1.953960 00:00:00_00:00:03 -> 00:00:00_00:00:04 LLC 176 I, N(R)=0, N(S)=0; DSAP NULL LSAP Individual, SSAP ISO Network Layer (unofficial?) Command
> 30 1.954280 -> 00:00:00_00:00:03 (RA) 802.11 14 Acknowledgement, Flags=o.......
> 31 1.954469 00:00:00_00:00:03 -> Broadcast LLC 76 S, func=RR, N(R)=0; DSAP NULL LSAP Individual, SSAP 0x1e Response
I would attribute this to new set of constructors for each time unit accepting double/float/uint/int, but I haven't yet found time to dig deeper to find out where the issues is triggered. I could probably do it if needed, but I bet someone experienced with wifi would find it much quicker than me if we rasied the issue on ns-developers.
Regards,
Vedran
Message from pdbj@mac.com
2013-11-25T20:23:20+00:00Peter Barnesurn:md5:a2f2319d2b3916fa6f61ef378c97fe87
I've played with alternative implementations of the unit conversion code and seen the same thing. In many cases I can trace it to a single bit difference in the underlying 64-bit time, representing one ns difference. In others it seems to get amplified dramatically; I've seen differences even higher than this, over 1 ms. I haven't been able to track down how this happens in the protocol implementation. I hypothesize that the single bit error occasionally causes an event to cross a boundary in a windowed protocol, such as the 1024 mus window in (is it 802.11?).
The other possibility, which I haven't looked at, is the bug in printing/reading of int64x64_t [bug 1786]. Is this used in generating test output? (Hmm, probably not in pcap, which is binary.)
If we are convinced this isn't a bug, we could regenerate the test references and be done. I don't see how your new units should affect old tests, however, suggesting there is a bug, or at least an interaction we don't understand.
Peter
On 2013/11/24 13:20:36, Vedran Miletić wrote:
> Updated, rebased on ns-3-dev (no changes needed though). Year is now 365 days,
> and week is gone. In-line doxygen documentation has been updated to provide
> information about year being 365 days, days being 24 hours etc.
>
> However, we have four failing tests (-regression ones with pcap traces). I did
> some inspection into devices-mesh-dot11s-regression, and it turns out that
> src/mesh/test/dot11s/hwmp-simplest-regression-test-0-1.pcap and generated pacp
> differ "only" in:
>
> < 17 1.942149 00:00:00_00:00:04 -> Broadcast LLC 76 S, func=RR, N(R)=0;
> DSAP NULL LSAP Individual, SSAP ISO Network Layer (unofficial?) Command
> …
>
> I would attribute this to new set of constructors for each time unit accepting
> double/float/uint/int, but I haven't yet found time to dig deeper to find out
> where the issues is triggered. I could probably do it if needed, but I bet
> someone experienced with wifi would find it much quicker than me if we rasied
> the issue on ns-developers.
Message from pdbj@mac.com
2013-11-25T20:23:31+00:00Peter Barnesurn:md5:29978c14b4480a9ebeb896276612b64e
https://codereview.appspot.com/14632043/diff/63001/src/core/model/nstime.h
File src/core/model/nstime.h (right):
https://codereview.appspot.com/14632043/diff/63001/src/core/model/nstime.h#newcode89
src/core/model/nstime.h:89: Y = 0, //!< year
//!< Julian calendar year, 365.25 days.
If you want to use common years, 365 days, we should discuss the reasons. On the one hand, it keeps "years" starting at midnight, on the other, it drifts wrt the civil calendar. I presume someone using years wants a lot of them, so the drift could be significant. But the main thing is to document and explain our choice.
https://codereview.appspot.com/14632043/diff/63001/src/core/model/nstime.h#newcode90
src/core/model/nstime.h:90: D = 1, //!< day
//!< Day, 24 hours, or 86,400 seconds.
https://codereview.appspot.com/14632043/diff/63001/src/core/model/nstime.h#newcode91
src/core/model/nstime.h:91: H = 2, //!< hour
//!< Hour, 60 minutes.
https://codereview.appspot.com/14632043/diff/63001/src/core/model/nstime.h#newcode92
src/core/model/nstime.h:92: MIN = 3, //!< minute
//!< Minute, 60 seconds.
https://codereview.appspot.com/14632043/diff/63001/src/core/model/nstime.h#newcode93
src/core/model/nstime.h:93: S = 4, //!< second
//!< SI second.
Message from rivanvx@gmail.com
2013-12-02T22:40:09+00:00Vedran Miletićurn:md5:058f3a84dcef7911f31d6f7d85de048d
On 2013/11/25 20:23:31, Peter Barnes wrote:
> https://codereview.appspot.com/14632043/diff/63001/src/core/model/nstime.h
> File src/core/model/nstime.h (right):
>
> https://codereview.appspot.com/14632043/diff/63001/src/core/model/nstime.h#newcode89
> src/core/model/nstime.h:89: Y = 0, //!< year
> //!< Julian calendar year, 365.25 days.
>
> If you want to use common years, 365 days, we should discuss the reasons. On
> the one hand, it keeps "years" starting at midnight, on the other, it drifts wrt
> the civil calendar. I presume someone using years wants a lot of them, so the
> drift could be significant. But the main thing is to document and explain our
> choice.
>
Main reason is to get "every larger unit is an integer multiple of smaller". Regarding lots of years, I expect most people will use years as a shorthand for 365 days, and my line of thought here (reliability engineering, specifically availability estimation) would be that you don't care too much if it's 100 years of 365 days or 365.25 days, as long as you know which one is used.
> https://codereview.appspot.com/14632043/diff/63001/src/core/model/nstime.h#newcode90
> src/core/model/nstime.h:90: D = 1, //!< day
> //!< Day, 24 hours, or 86,400 seconds.
>
> https://codereview.appspot.com/14632043/diff/63001/src/core/model/nstime.h#newcode91
> src/core/model/nstime.h:91: H = 2, //!< hour
> //!< Hour, 60 minutes.
>
> https://codereview.appspot.com/14632043/diff/63001/src/core/model/nstime.h#newcode92
> src/core/model/nstime.h:92: MIN = 3, //!< minute
> //!< Minute, 60 seconds.
>
> https://codereview.appspot.com/14632043/diff/63001/src/core/model/nstime.h#newcode93
> src/core/model/nstime.h:93: S = 4, //!< second
> //!< SI second.
Agree with all of these, will fix them before pushing.
Message from rivanvx@gmail.com
2013-12-02T22:41:14+00:00Vedran Miletićurn:md5:21885f9badd49460ac09ec4e26c351f4
On 2013/11/25 20:23:20, Peter Barnes wrote:
> I've played with alternative implementations of the unit conversion code and
> seen the same thing. In many cases I can trace it to a single bit difference in
> the underlying 64-bit time, representing one ns difference. In others it seems
> to get amplified dramatically; I've seen differences even higher than this, over
> 1 ms. I haven't been able to track down how this happens in the protocol
> implementation. I hypothesize that the single bit error occasionally causes an
> event to cross a boundary in a windowed protocol, such as the 1024 mus window in
> (is it 802.11?).
>
> The other possibility, which I haven't looked at, is the bug in printing/reading
> of int64x64_t [bug 1786]. Is this used in generating test output? (Hmm,
> probably not in pcap, which is binary.)
>
> If we are convinced this isn't a bug, we could regenerate the test references
> and be done. I don't see how your new units should affect old tests, however,
> suggesting there is a bug, or at least an interaction we don't understand.
>
I will check the "new set of constructors causes this hypothesis", and we can proceed from there.
Regards,
Vedran
Message from tomh.org@gmail.com
2013-12-03T15:19:50+00:00Tom Hendersonurn:md5:385d94ced8273a78c9ccfe2a8a1daaef
On 2013/11/25 20:23:20, Peter Barnes wrote:
> I've played with alternative implementations of the unit conversion code and
> seen the same thing. In many cases I can trace it to a single bit difference in
> the underlying 64-bit time, representing one ns difference. In others it seems
> to get amplified dramatically; I've seen differences even higher than this, over
> 1 ms. I haven't been able to track down how this happens in the protocol
> implementation. I hypothesize that the single bit error occasionally causes an
> event to cross a boundary in a windowed protocol, such as the 1024 mus window in
> (is it 802.11?).
>
> The other possibility, which I haven't looked at, is the bug in printing/reading
> of int64x64_t [bug 1786]. Is this used in generating test output? (Hmm,
> probably not in pcap, which is binary.)
>
> If we are convinced this isn't a bug, we could regenerate the test references
> and be done. I don't see how your new units should affect old tests, however,
> suggesting there is a bug, or at least an interaction we don't understand.
I tested this a bit to make sure that the test differences were not due to random variable issues (static initialization order), and this seems to be something different. Just FYI...
>
> Peter
>
> On 2013/11/24 13:20:36, Vedran Miletić wrote:
> > Updated, rebased on ns-3-dev (no changes needed though). Year is now 365 days,
> > and week is gone. In-line doxygen documentation has been updated to provide
> > information about year being 365 days, days being 24 hours etc.
> >
> > However, we have four failing tests (-regression ones with pcap traces). I did
> > some inspection into devices-mesh-dot11s-regression, and it turns out that
> > src/mesh/test/dot11s/hwmp-simplest-regression-test-0-1.pcap and generated pacp
> > differ "only" in:
> >
> > < 17 1.942149 00:00:00_00:00:04 -> Broadcast LLC 76 S, func=RR, N(R)=0;
> > DSAP NULL LSAP Individual, SSAP ISO Network Layer (unofficial?) Command
> > …
> >
> > I would attribute this to new set of constructors for each time unit accepting
> > double/float/uint/int, but I haven't yet found time to dig deeper to find out
> > where the issues is triggered. I could probably do it if needed, but I bet
> > someone experienced with wifi would find it much quicker than me if we rasied
> > the issue on ns-developers.
Message from rivanvx@gmail.com
2013-12-06T18:43:59+00:00Vedran Miletićurn:md5:fba1080f1ab10518aaef39ed19e67219
Overloaded constructors are the problem. Removed them; no fails. Apparently we have some code that relies on some automatic int->float or float->int conversion.
I removed overloads, added the documentation suggestions made by Peter and pushed, since I consider this now to be no worse than we had in 3.18.
We can look into fixing problematic code later, and then add overloads if they are needed.