Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(88)

Issue 6821106: [Bug 954] Time::SetResolution (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
9 years, 6 months ago by Peter Barnes
Modified:
7 years, 4 months ago
CC:
ns-3-reviews_googlegroups.com
Visibility:
Public.

Description

[Bug 954] Support Time::SetResolution in user code, v2 Revised patch addressing issues from Dec 2012. This is roughly what was proposed at the ns-3 Developer meeting in March 2013. Notable changes from that discussion: - Can't initialize a class static data member in the header, without C++11. Instead, I added a StaticInit () function, and force every compilation unit to invoke it. - This means we can dispense with the static bool m_markingTime, and just use the non-nullness of the set pointer, which I renamed g_markingTimes. - In my testing, it appears that the CriticalSection is unnecessary (no where else in src/core uses it either) - The real problem appears to be static initialization order between Time and Simulator, triggered by the call in Time to Simulator::Schedule. I adopted Mathieu's suggestion of adding an explicit call in Simulator::Run to Time::ClearMarkedTimes. There are still some lingering style issues (whitespace), but I didn't want to clutter the code review with more noise. [Bug 954] Support Time::SetResolution in user code. This patch logs the address of every Time, until SetResolution is called. On SetResolution, every existing Time is converted to the new resolution, and the log is deleted. This allows users to SetResolution exactly once. I also considered lazy conversion, adding a unit to Time, and checking each Time as it was accessed, but this requires constant checks on each access. Assuming that Times are accessed many times (per instance), confining the additional work to construction/destruction seems like a better approach. Note that once Time::SetResolution is called, the data structure that tracks live Time objects (a std::set) is deleted, so we only support setting the resolution once. This reduces subsequent overhead to a function call and test (and only on construction/destruction), which should be very small.

Patch Set 1 : Corrected patch set #

Total comments: 12

Patch Set 2 : Revised patch #

Patch Set 3 : [Bug 954] Support Time::SetResolution in user code, v2 #

Total comments: 2

Patch Set 4 : [Bug 954] Support Time::SetResolution in user code, v3 #

Patch Set 5 : Fix bug affecting conversion of Time::Min/Time::Max during Time::SetResolution #

Patch Set 6 : Rebased on r10104 8705e2943d7d #

Patch Set 7 : Fix showpos bug, rebase on r10106 41f46a8e5a96 #

Patch Set 8 : Protect g_markingTimes with a mutex. #

Unified diffs Side-by-side diffs Delta from patch set Stats (+507 lines, -83 lines) Patch
M doc/tutorial/source/conceptual-overview.rst View 1 2 3 5 1 chunk +15 lines, -0 lines 0 comments Download
M examples/tutorial/first.cc View 2 5 1 chunk +1 line, -0 lines 0 comments Download
M src/core/model/nstime.h View 1 2 3 4 5 19 chunks +226 lines, -43 lines 0 comments Download
M src/core/model/simulator.cc View 1 2 5 1 chunk +1 line, -0 lines 0 comments Download
M src/core/model/time.cc View 1 2 3 4 5 6 7 5 chunks +243 lines, -30 lines 0 comments Download
M src/core/model/unused.h View 1 2 3 5 1 chunk +10 lines, -0 lines 0 comments Download
M src/core/test/time-test-suite.cc View 1 2 3 5 3 chunks +11 lines, -10 lines 0 comments Download

Messages

Total messages: 47
Peter Barnes
Hello Folks, Here is a solution to Bug 954: https://www.nsnam.org/bugzilla/show_bug.cgi?id=954 Code review: http://codereview.appspot.com/6821106/ Peter
9 years, 6 months ago (2012-11-11 07:30:28 UTC) #1
Vedran Miletić
On 2012/11/11 07:30:28, Peter Barnes wrote: > Hello Folks, > > Here is a solution ...
9 years, 6 months ago (2012-11-14 15:03:50 UTC) #2
Tommaso Pecorella
I have to confess I didn't check the code very deeply, but I trust 100% ...
9 years, 6 months ago (2012-11-14 16:56:40 UTC) #3
Peter Barnes
I had the same thought. The assert is at time.cc:233. On 2012/11/14 16:56:40, Tommaso Pecorella ...
9 years, 6 months ago (2012-11-14 23:30:23 UTC) #4
Peter Barnes
Operator error. The correct patch is now on code review.appsppot.com On 2012/11/14 15:03:50, Vedran Miletić ...
9 years, 6 months ago (2012-11-15 06:04:32 UTC) #5
Tommaso Pecorella
On 2012/11/15 06:04:32, Peter Barnes wrote: > Operator error. The correct patch is now on ...
9 years, 6 months ago (2012-11-15 21:31:16 UTC) #6
Peter Barnes
Working at weapons lab, we have ingrained in us a Russia proverb: Trust, but verify... ...
9 years, 6 months ago (2012-11-16 19:39:28 UTC) #7
Peter Barnes
Which is why I need code review... On 2012/11/15 21:31:16, Tommaso Pecorella wrote: > Seems ...
9 years, 6 months ago (2012-11-16 19:39:53 UTC) #8
Vedran Miletić
+1 on what Tommaso said. This is a great feature. Aside from coding style and ...
9 years, 6 months ago (2012-11-17 19:13:27 UTC) #9
Peter Barnes
I'm happy to make changes. What issues do you see? On 2012/11/17 19:13:27, Vedran Miletić ...
9 years, 6 months ago (2012-11-19 17:36:33 UTC) #10
Vedran Miletić
Hi Peter, this is it. Some additional suggestions on assert and documetation are included. Regards, ...
9 years, 6 months ago (2012-11-19 23:23:11 UTC) #11
Peter Barnes
https://codereview.appspot.com/6821106/diff/8001/src/core/model/nstime.h File src/core/model/nstime.h (right): https://codereview.appspot.com/6821106/diff/8001/src/core/model/nstime.h#newcode78 src/core/model/nstime.h:78: * in a 64 bit integer, whose interpretation will ...
9 years, 5 months ago (2012-11-29 04:02:21 UTC) #12
Mathieu Lacage
What is really missing, is a performance analysis of the impact of this code. i.e., ...
8 years, 11 months ago (2013-06-11 07:03:57 UTC) #13
Peter Barnes
On 2013/06/11 07:03:57, Mathieu Lacage wrote: > What is really missing, is a performance analysis ...
8 years, 11 months ago (2013-06-14 00:50:04 UTC) #14
Mathieu Lacage
+1 for merging if you can either remove the extra operator = I spotted earlier ...
8 years, 10 months ago (2013-07-04 07:09:36 UTC) #15
Vedran Miletić
Peter, can we get rid of this? ./ns3/nstime.h: At global scope: ./ns3/nstime.h:619:13: error: ‘ns3::g_TimeStaticInit’ defined ...
8 years, 10 months ago (2013-07-04 07:52:41 UTC) #16
Peter Barnes
On 2013/07/04 07:52:41, Vedran Miletić wrote: > Peter, can we get rid of this? > ...
8 years, 10 months ago (2013-07-04 14:32:14 UTC) #17
Peter Barnes
https://codereview.appspot.com/6821106/diff/20001/src/core/model/nstime.h File src/core/model/nstime.h (right): https://codereview.appspot.com/6821106/diff/20001/src/core/model/nstime.h#newcode110 src/core/model/nstime.h:110: inline Time &operator = (const int64_t &value) On 2013/06/11 ...
8 years, 10 months ago (2013-07-04 14:33:56 UTC) #18
Mathieu Lacage
On 2013/07/04 14:33:56, Peter Barnes wrote: > https://codereview.appspot.com/6821106/diff/20001/src/core/model/nstime.h > File src/core/model/nstime.h (right): > > https://codereview.appspot.com/6821106/diff/20001/src/core/model/nstime.h#newcode110 ...
8 years, 10 months ago (2013-07-10 13:25:19 UTC) #19
Peter Barnes
8 years, 10 months ago (2013-07-16 01:05:01 UTC) #20
Peter Barnes
On 2013/07/16 01:05:01, Peter Barnes wrote: Well, that didn't go smoothly. This is the v2 ...
8 years, 10 months ago (2013-07-16 01:15:05 UTC) #21
Peter Barnes
I think all review comments have been addressed. Okay to commit? I'd like to get ...
8 years, 10 months ago (2013-07-18 23:57:58 UTC) #22
Vedran Miletić
On 2013/07/18 23:57:58, Peter Barnes wrote: > I think all review comments have been addressed. ...
8 years, 10 months ago (2013-07-22 09:51:14 UTC) #23
Vedran Miletić
On 2013/07/22 09:51:14, Vedran Miletić wrote: > On 2013/07/18 23:57:58, Peter Barnes wrote: > > ...
8 years, 10 months ago (2013-07-23 09:31:05 UTC) #24
Tommaso Pecorella
On 2013/07/23 09:31:05, Vedran Miletić wrote: > WAIT! Something nasty is happening with Time in ...
8 years, 10 months ago (2013-07-23 10:50:12 UTC) #25
Vedran Miletić
On 2013/07/23 10:50:12, Tommaso Pecorella wrote: > On 2013/07/23 09:31:05, Vedran Miletić wrote: > > ...
8 years, 10 months ago (2013-07-23 11:04:02 UTC) #26
Peter Barnes
8 years, 10 months ago (2013-07-24 21:14:20 UTC) #27
Peter Barnes
Tommaso and Vedran: thanks for finding this, and the excellent examples. The issue occurs when ...
8 years, 10 months ago (2013-07-24 21:15:57 UTC) #28
Peter Barnes
Tommaso and Vedran: thanks for finding this, and the excellent examples. The issue occurs when ...
8 years, 10 months ago (2013-07-24 21:16:03 UTC) #29
Vedran Miletić
Peter, Tommaso et. al., there is more. Time t0 = Seconds (-1); NS_LOG_UNCOND (t0.GetSeconds ()); ...
8 years, 10 months ago (2013-07-25 11:01:10 UTC) #30
barnes26_llnl.gov
Wow, that's been lurking for a while, since Time::Information::factor was introduced as uint64_t in 92a84235d8f2, ...
8 years, 10 months ago (2013-07-25 19:17:36 UTC) #31
Vedran Miletić
On 2013/07/25 19:17:36, barnes26_llnl.gov wrote: > Wow, that's been lurking for a while, since Time::Information::factor ...
8 years, 10 months ago (2013-07-26 12:08:15 UTC) #32
Vedran Miletić
Peter, I can't apply this, sorry. Could you post the entire patch merged together and ...
8 years, 9 months ago (2013-07-27 14:33:34 UTC) #33
Peter Barnes
8 years, 9 months ago (2013-07-31 18:54:15 UTC) #34
Peter Barnes
Patch Set 6 : Rebased on r10104 8705e2943d7d Peter On Jul 27, 2013, at 7:33 ...
8 years, 9 months ago (2013-07-31 18:59:41 UTC) #35
Vedran Miletić
On 2013/07/31 18:59:41, Peter Barnes wrote: > Patch Set 6 : Rebased on r10104 8705e2943d7d ...
8 years, 9 months ago (2013-07-31 20:40:06 UTC) #36
Vedran Miletić
On 2013/07/31 20:40:06, Vedran Miletić wrote: > On 2013/07/31 18:59:41, Peter Barnes wrote: > > ...
8 years, 9 months ago (2013-08-01 10:32:18 UTC) #37
Peter Barnes
8 years, 9 months ago (2013-08-02 00:46:10 UTC) #38
Vedran Miletić
On 2013/08/02 00:46:10, Peter Barnes wrote: Peter, I'm sorry to report that I get crashes ...
8 years, 9 months ago (2013-08-04 08:44:37 UTC) #39
Vedran Miletić
On 2013/08/04 08:44:37, Vedran Miletić wrote: > On 2013/08/02 00:46:10, Peter Barnes wrote: > > ...
8 years, 9 months ago (2013-08-05 16:20:30 UTC) #40
Vedran Miletić
On 2013/08/05 16:20:30, Vedran Miletić wrote: > On 2013/08/04 08:44:37, Vedran Miletić wrote: > > ...
8 years, 9 months ago (2013-08-12 22:05:14 UTC) #41
Vedran Miletić
On 2013/08/12 22:05:14, Vedran Miletić wrote: > On 2013/08/05 16:20:30, Vedran Miletić wrote: > > ...
8 years, 9 months ago (2013-08-12 22:26:05 UTC) #42
Peter Barnes
8 years, 9 months ago (2013-08-15 00:04:57 UTC) #43
Peter Barnes
I can statistically reproduce python crashes in optimized builds. Specifically, these examples: examples/routing/simple-routing-ping6.py examples/tutorial/first.py examples/wireless/wifi-ap.py ...
8 years, 9 months ago (2013-08-15 00:34:12 UTC) #44
Vedran Miletić
On 2013/08/15 00:34:12, Peter Barnes wrote: > I can statistically reproduce python crashes in optimized ...
8 years, 9 months ago (2013-08-15 13:16:31 UTC) #45
Peter Barnes
On 2013/08/15 13:16:31, Vedran Miletić wrote: > Same here. Does this have a measurable performance ...
8 years, 9 months ago (2013-08-17 00:54:11 UTC) #46
Peter Barnes
8 years, 9 months ago (2013-08-22 00:04:58 UTC) #47
> On 2013/08/15 13:16:31, Vedran Miletić wrote:
> > Same here. Does this have a measurable performance impact?
> 
> I used the same benchmarks described in comment 14, above.
> 
> 
> 1.  Microbenchmark
> 
> After SetResolution,  I measure a difference of 2.1 ± 0.3 ns/ iteration 
> (patch is slower).  Comment 14 had 0.6 ns/iteration, but there shouldn't be 
> any difference, so this is unexplained.

I reran the benchmark with -d optimized in both cases, and the difference is
0.54 ± 0.13 ns/iteration, consistent with the prior result in comment 14, as
expected.  I must not have had one of the repos in optimized state for the
results
quoted in comment 46.

> Before SetResolution, the mutex-protected version takes 1.94 ± 0.05
> mus/iteration, compared to 0.123 mus/iteration in comment 14.  However, now
ns-3-dev takes
> 19.3 ± 0.6 ns/event; in comment 14 it took 1.2 ns/event.

With both repos in optimized, mutex-protected version takes 220 ± 1
ns/iteration,
compared to 123 ns/iteration with Time marking, but no mutex.  Prior to the
patch ns-3-dev takes
1.7 ± 0.1 ns/iteration, now consistent with comment 14.

> 2.  Macrobenchmark
> 
> After SetResolution I measure a difference of -3.4 ± 2.0 ns/event, consistent
> with zero (patch is faster).
> 
> Before SetResolution I measure a difference of -414 ± 56 ns/event, again 
> patch is faster.

With both repos in optimized the difference is 2.0 ± 1.1 ns/event (mutex slower
than ns-3-dev), consistent with zero and the result in comment 14.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b