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

fills in profiler timer fallback #1350

Merged
merged 1 commit into from Jul 13, 2015

Conversation

Projects
None yet
4 participants
@Azaezel
Contributor

Azaezel commented Jul 11, 2015

as per remmed out lines in other samples. see #1349 for report, and https://gist.github.com/Azaezel/2aafb88cf0d642418051 for result

@crabmusket

This comment has been minimized.

Show comment
Hide comment
@crabmusket

crabmusket Jul 12, 2015

Contributor

It's not exactly high-resolution bit it's better than nothing! Though, I wonder if we should look at this C++11 solution. Oh, but of course VS2012 doesn't implement it correctly. Workaround.

Contributor

crabmusket commented Jul 12, 2015

It's not exactly high-resolution bit it's better than nothing! Though, I wonder if we should look at this C++11 solution. Oh, but of course VS2012 doesn't implement it correctly. Workaround.

@JeffProgrammer

This comment has been minimized.

Show comment
Hide comment
@JeffProgrammer

JeffProgrammer Jul 12, 2015

Contributor

Why not just use each platform's specific high precision timer? Windows: QueryPerformanceCounter/Frequency, Linux/unix: clock_gettime time.h, OSX: mach timer

That way it stays with the C++03 standard.

Also...SDL has a platform agnostic high precision timer, just saying....

Contributor

JeffProgrammer commented Jul 12, 2015

Why not just use each platform's specific high precision timer? Windows: QueryPerformanceCounter/Frequency, Linux/unix: clock_gettime time.h, OSX: mach timer

That way it stays with the C++03 standard.

Also...SDL has a platform agnostic high precision timer, just saying....

@dottools

This comment has been minimized.

Show comment
Hide comment
@dottools

dottools Jul 12, 2015

std::chrono::high_resolution_clock is really bad and should not be used. For one you don't know if it's really an alias to just std::chrono::system_clock, std::chrono::steady_clock, or something else as explained on CPP Reference. Second neither of the first two clock sources are suitable for benchmarking/profiling code. You want a timer source that only increments on thread CPU usage, which there is not option to choose from in C++11.

Similar to what Jeff H. has said. We should be implementing platform specific timers that are known to be correct or good enough timer sources for profiling.

  • For Linux (and Android) that's clock_gettime(CLOCK_THREAD_CPUTIME_ID, &ts), example.
  • For Windows that's GetThreadTimes and combining lpKernelTime and lpUserTime values: GetThreadTimes() documentation.
  • No idea about Mac OS X.

dottools commented Jul 12, 2015

std::chrono::high_resolution_clock is really bad and should not be used. For one you don't know if it's really an alias to just std::chrono::system_clock, std::chrono::steady_clock, or something else as explained on CPP Reference. Second neither of the first two clock sources are suitable for benchmarking/profiling code. You want a timer source that only increments on thread CPU usage, which there is not option to choose from in C++11.

Similar to what Jeff H. has said. We should be implementing platform specific timers that are known to be correct or good enough timer sources for profiling.

  • For Linux (and Android) that's clock_gettime(CLOCK_THREAD_CPUTIME_ID, &ts), example.
  • For Windows that's GetThreadTimes and combining lpKernelTime and lpUserTime values: GetThreadTimes() documentation.
  • No idea about Mac OS X.

@crabmusket crabmusket added the Bug label Jul 13, 2015

@crabmusket

This comment has been minimized.

Show comment
Hide comment
@crabmusket

crabmusket Jul 13, 2015

Contributor

Roger that @dottools, thanks for the info. @Azaezel I'm going to merge this for now until we can work on a longer-term solution. I'm happy to take that on, unless you need it today ;).

Contributor

crabmusket commented Jul 13, 2015

Roger that @dottools, thanks for the info. @Azaezel I'm going to merge this for now until we can work on a longer-term solution. I'm happy to take that on, unless you need it today ;).

@crabmusket crabmusket added this to the 3.8 milestone Jul 13, 2015

crabmusket added a commit that referenced this pull request Jul 13, 2015

@crabmusket crabmusket merged commit ec63398 into GarageGames:development Jul 13, 2015

@Azaezel Azaezel deleted the Azaezel:timingIsEverything branch Jul 23, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment