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

Use doubles for startTime and endTime #67

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jaredp
Copy link

@jaredp jaredp commented Jul 29, 2015

don’t lose precision by doing integer arithmetic

@jaredp
Copy link
Author

jaredp commented Jul 31, 2015

It looks like the resolution of the timer is a factor of 10 less granular exclusively on node.js v12 in 64-bit. Does anyone know why?

@3y3
Copy link
Member

3y3 commented Jul 31, 2015

Hello, @jaredp , thank you for contribution.
I need some time to test this on compatibility with DevTools frontend in node-inspector.
About test - it is very speculative. Testing in cloud containers like Travis and Appveyor can't guaranty what this async operation will be finished in time below 3*delay.
I prefer to see here only least checker.

@jaredp
Copy link
Author

jaredp commented Aug 3, 2015

Hi @3y3, thanks for taking a look.
The test failed because profiler reported 1.5469829999999547 seconds for what should have been a delay of 0.15 seconds. We should expect on a box that's not too busy to see an elapsed time a little over 0.15 seconds but likely less than 0.16 seconds; 0.15469... seconds would be perfectly expected. On an AppVeyor box busy with other things, we can expect to see an elapsed time a coefficient more, but this is exactly an order of magnitude, so I think the reporting is accurate, and for some reason the resolution is different in this particular config.

@3y3
Copy link
Member

3y3 commented Aug 4, 2015

@jaredp , 1.5469829999999547 is a real timeout for this case. In other words iteration on AppVeyor and on Travis can be very slow sometimes. We can't expect what elapsed time will be in interval 150ms < elapsedTime < 160ms. We also can't expect 150ms < elapsedTime < 150h.
I prefer to merge this change without new test.

don’t lose precision by doing integer arithmetic
@jaredp
Copy link
Author

jaredp commented Aug 4, 2015

Okay, removed the < 350ms test

@jaredp
Copy link
Author

jaredp commented Aug 11, 2015

ping

@3y3 3y3 force-pushed the master branch 2 times, most recently from 7f49e61 to d0b888b Compare September 20, 2015 16:32
@jaredp
Copy link
Author

jaredp commented Sep 23, 2015

bump

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.

None yet

2 participants