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

Chrome trace timestamp should be in microseconds not seconds. #8600

Merged
merged 3 commits into from
Nov 21, 2022

Conversation

sklam
Copy link
Member

@sklam sklam commented Nov 15, 2022

Fix #8599.

@sklam sklam added 3 - Ready for Review Effort - short Short size effort needed labels Nov 15, 2022
@sklam sklam marked this pull request as ready for review November 15, 2022 18:10
Copy link
Contributor

@stuartarchibald stuartarchibald left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the patch.

A potentially important side discussion is needed here as I don't think this is necessarily always a question of scaling microseconds to seconds. From the code, it looks like the timer used in the event module is based on timeit.default_timer. From the python docs, timeit.default_timer is always time.perf_counter, which doesn't seem to guarantee the unit of the returned value, it's just "the clock with the highest available resolution". This means the resolution needs to be known to work out an appropriate scaling factor to get to the unit of seconds.

I think there's two options:

  1. Use time.perf_counter_ns for timing, time would then always be reported in nanoseconds and scaling to seconds would always be via a constant factor. This also has the advantage of not losing precision through the use of a float type as the returned value.
  2. Continue to use timeit.default_timer i.e. time.perf_counter. Then query time.get_clock_info('perf_counter') to get the resolution of the timer, the inverse of which is the scaling to get to the unit of seconds.

Any thoughts @sklam ?

numba/core/event.py Outdated Show resolved Hide resolved
@stuartarchibald stuartarchibald added 4 - Waiting on author Waiting for author to respond to review and removed 3 - Ready for Review labels Nov 16, 2022
@sklam
Copy link
Member Author

sklam commented Nov 16, 2022

@stuartarchibald, even though timeit.default_timer has unknown resolution but it is always in the unit of seconds. The new doc now say defaut_timer is always time.perf_counter which has Return the value (in fractional seconds) of a performance counter.

The tracing format expect a float type in microseconds unit anyway so i don't think the loss of precision matter that much. And, Numba is not that fast.

Co-authored-by: stuartarchibald <stuartarchibald@users.noreply.github.com>
@sklam sklam added 4 - Waiting on reviewer Waiting for reviewer to respond to author and removed 4 - Waiting on author Waiting for author to respond to review labels Nov 16, 2022
numba/core/event.py Outdated Show resolved Hide resolved
@stuartarchibald
Copy link
Contributor

@stuartarchibald, even though timeit.default_timer has unknown resolution but it is always in the unit of seconds. The new doc now say defaut_timer is always time.perf_counter which has Return the value (in fractional seconds) of a performance counter.

The tracing format expect a float type in microseconds unit anyway so i don't think the loss of precision matter that much. And, Numba is not that fast.

Thanks! Following some OOB discussion, turns out this is all ok. The issue is that the tracer output needs to be in microseconds and the clocks are reporting in seconds even if not explicitly noted, so the scale factor is constant and there's no resolution issue or otherwise!

Copy link
Contributor

@stuartarchibald stuartarchibald left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the updates, changes look good. @guilhermeleobas did you perhaps want to test this as in #8599 ?

@stuartarchibald stuartarchibald added this to the Numba 0.57 RC milestone Nov 16, 2022
@guilhermeleobas
Copy link
Contributor

Thanks, I can confirm the fix solves the original issue.
image

@stuartarchibald
Copy link
Contributor

Thanks for confirming @guilhermeleobas !

@stuartarchibald stuartarchibald added 5 - Ready to merge Review and testing done, is ready to merge and removed 4 - Waiting on reviewer Waiting for reviewer to respond to author labels Nov 21, 2022
@sklam sklam merged commit 09c2c05 into numba:main Nov 21, 2022
@sklam sklam deleted the fix/8599 branch November 21, 2022 23:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
5 - Ready to merge Review and testing done, is ready to merge Effort - short Short size effort needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Numba event mechanism is recording seconds as microseconds?
3 participants