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

Timer Initialization and Accuracy Fix #1356

Merged
merged 1 commit into from Aug 17, 2018
Merged

Timer Initialization and Accuracy Fix #1356

merged 1 commit into from Aug 17, 2018

Conversation

RobertBColton
Copy link
Contributor

@RobertBColton RobertBColton commented Aug 16, 2018

The generalization of the main loop for SDL introduced a regression I finally caught reviewing my benchmark in #1289. The timing variables should be initialized just before input and before ANY user events (e.g, game start, room start, or the create event in my case) are fired at all, otherwise get_timer is completely inaccurate.

This occurred on Win32 where we don't query whether to use the high quality performance counter until the timing is initialized. All of the microsecond times I reported in my benchmark for #1289 are basically inaccurate to within 1/1000th of a microsecond. I am going to have to redo the entire benchmark now with this patch to see the actual difference.

I caught the mistake by wondering why none of my changes to the new model creation (in an effort to speed it up again) were affecting the results at all. I continued moving the get_timer lower and lower until I was timing the creation of a single slice of cubes. It was then reporting 0 or 15,000, but nothing in between. With this fix applied it is very very accurate and always reports differences of a microsecond or two.

Benchmark Update

Because of this issue, I decided I must record the model creation times from the benchmark in #1289 again. The results I am going to share here are relative to that pull request, not this one, because I did a hard reset and retroactively applied this patch. I record for each system the best time out of three tries.

The results basically indicate that, yeah, #1289 did generally increase the create time despite the other advantages. But with this PR going forward, I can accurately measure any optimizations I make to speed the model creation back up again. I can't do that without this change.

Branch Create time PR (µs) Create time Master (µs)
ENIGMA GL1 113138 71770
ENIGMA GL3 113388 77767
ENIGMA DX9 116423 70992

@RobertBColton RobertBColton added the Platforms Win32, Xlib, Cocoa, SDL and other target platforms. label Aug 16, 2018
@RobertBColton RobertBColton changed the title Timer Initialization Fix Timer Initialization and Accuracy Fix Aug 16, 2018
@codecov
Copy link

codecov bot commented Aug 16, 2018

Codecov Report

Merging #1356 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1356   +/-   ##
=======================================
  Coverage   15.46%   15.46%           
=======================================
  Files         167      167           
  Lines       17386    17386           
=======================================
  Hits         2688     2688           
  Misses      14698    14698
Impacted Files Coverage Δ
ENIGMAsystem/SHELL/Platforms/General/PFmain.cpp 81.81% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d490b50...06322d4. Read the comment docs.

@RobertBColton RobertBColton merged commit 523053e into master Aug 17, 2018
@RobertBColton RobertBColton deleted the timer-init-fix branch August 17, 2018 00:56
JoshDreamland pushed a commit that referenced this pull request Nov 10, 2018
This way they are accurate to use in create/game start/room start events.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Platforms Win32, Xlib, Cocoa, SDL and other target platforms.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants