HOMS-OSS / ruperf

A smart and sophisticated performance analysis crab.
GNU General Public License v2.0
7 stars 5 forks source link

Serialize timers. #118

Closed Sl1mb0 closed 3 years ago

Sl1mb0 commented 3 years ago

This is an attempt to make the timing information more accurate. This is accomplished by using SystemTime in both of the parent and child processes to compare against; and flushing writers to make sure that readers finish reading.

I have run it against perf stat, and while CPUs utilized is not accurate yet, task-clocks is much more closer to the result of perf than before.

Sl1mb0 commented 3 years ago

I added an assertion so that we know the two instances in time will be in the correct order. I also changed where I started the counters, because it didn't make sense to start them after we tell the child they may begin since these processes will execute asynchronously unless explicitly told otherwise.

Boursler commented 3 years ago

Yes, I particularly like the change where you reorganize where the timers start; nice catch.

I really don't think there should be a panicking assertion if SystemTime returns two times that do not show a monotonic increase; this strikes me as poor user experience, especially since the problem would go back to the system timer working as it should.

One of the reasons SystemTime is not monotonic is because cores can be out of sync; another is that SystemTime is configurable/adjustable on a System by either system user or automated tools (which we won't be able to control at all). This is why I wanted to rely on Instant which does provide a monotonic guarantee (ideal for timers) and is not reliant on a configurable timer on the system.

Sl1mb0 commented 3 years ago

Maybe compare both and use the smaller of the two?

Boursler commented 3 years ago

No, that is not how it works. System time measures what time the system believes the real world time is. Instant time measures time with the aid of a timer built into the machines hardware. The only reason to use system time is if you care about the real world time. Otherwise, all that call does is introduce the possibility that the real world time is being set to a new value. Comparing the two is comparing two unlike things. As an example for why this is bad, consider benchmarking a utility that changes system time. Ruperf would not work in this case if it measures system time.

The underlying Unix implementation in Rust for these two calls uses https://linux.die.net/man/3/clock_gettime with Instant returning the CLOCK_MONOTONIC and SystemTime returning the CLOCK_REALTIME. Note that the manpage specifies that privileges are needed in order to set the CLOCK_REALTIME clock. This is because, as I said, the SystemTime clock is modifiable. See this fairly in-depth discussion. Here is also a Stack Overflow discussion that directly addresses implementing counters with either CLOCK_REALTIME or CLOCK_MONOTONIC .

Sl1mb0 commented 3 years ago

Alright I'm going to fix it to Instant instead.

Sl1mb0 commented 3 years ago

In case you're wondering, I included comparisons between the implementation in this PR and this example:

read()
let now = Instant::now();
wait()
let t = now.elapsed()

I ran what's in the example above and what's currently in this PR 7 times each and then averaged their CPUs utilized values.

Averages

Example Pull Request
1.005 .988