Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Converted timing mechanisms to use Boost.Chrono #234

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

BentSm
Copy link
Contributor

@BentSm BentSm commented Feb 14, 2017

This replaces all timing functionality with Boost.Chrono. This should work fine anywhere Boost.Chrono works; however, I was unable to determine if there are any systems that need to be handled specially (although to do so would seem to be contrary to the purpose of these changes). (See https://github.com/POV-Ray/povray/projects/3#card-991550.)

@c-lipka
Copy link
Contributor

c-lipka commented Feb 14, 2017

I had contemplated going for Boost.Chrono earlier, but decided against it, due to the following statement:

"The underlying clocks provided by operating systems are subject to many seemingly arbitrary policies and implementation irregularities. That's a polite way of saying they tend to be flakey, and each operating system or even each clock has its own cruel and unusual forms of flakiness. Don't bet the farm on their accuracy, unless you have become deeply familiar with exactly what the specific operating system is guaranteeing, which is often very little."

In other words, by using Boost.Chrono we give up control over which timers are actually used, and may get sub-optimal timers for our purposes. For instance, one "very standard" way to access process time on Unix is only good for about half an hour of CPU time, which is obviously far too little for hour-long renders.

We therefore use the platform-specific timers directly, so that we can prioritize our choice of timer.

@c-lipka
Copy link
Contributor

c-lipka commented Feb 14, 2017

For clarification: I'm not intending to say Boost.Chrono is a bad choice -- I'm intending to say that right now I have too vague a picture of Boost.Chrono to happily pull your request; I'd need to see a very thorough in-depth review of the matter first.

@BentSm
Copy link
Contributor Author

BentSm commented Feb 14, 2017

The clocks used in Boost.Chrono are detailed at http://www.boost.org/doc/libs/1_63_0/doc/html/chrono/appendices.html#chrono.appendices.implementation. I had originally added Boost.Chrono as another option to allow thread timing on OS X, over in https://github.com/BentSm/povray/tree/condensed-chrono; the project card was what inspired me to do a total conversion. Boost.Chrono does take care of range issues, by the way (see http://www.boost.org/doc/libs/1_63_0/doc/html/chrono/users_guide.html#chrono.users_guide.tutorial.duration).

@c-lipka
Copy link
Contributor

c-lipka commented Feb 14, 2017

I've had a closer look at the automated build test results by now:

From the AppVeyor logs, it seems that Boost.Chrono doesn't work in header-only mode on Windows machines, and needs a Visual Studio project set up (akin to Boost.Thread, Boost.System and Boost.DateTime) to build it as a library. That means some work to do, but presumably no serious obstacle.

As for the Travis CI build failure, it highlights a more serious issue: Boost.Chrono requires boost 1.47 or later, while our current official requirement is boost 1.38. While we might consider bumping the requirement, some long-term-support Linux distros are still at versions prior to 1.47; most notably, Ubuntu 12.04 LTE (which is what Travis CI runs) happens to be at 1.46, just one version short. Requiring users of such distros to up their boost version would be possible, but I guess we'd want to avoid that.

I'd like to encourage you to keep your pull request alive, as I think it might come in handy in the future, but it may take a while before we go ahead and pull it.

@BentSm
Copy link
Contributor Author

BentSm commented Feb 19, 2017

Will do. (Sorry about the delay in response.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants