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

feat(performance-metrics): add FunctionTimer #14834

Closed
wants to merge 10 commits into from

Conversation

DerekMaggio
Copy link
Contributor

@DerekMaggio DerekMaggio commented Apr 8, 2024

Overview

Add FunctionTimer context manager

Test Plan

  • Cover as many happy-path and edge cases as possible to ensure that usage of FunctionTimer does not interfere with underlying code

Changelog

  • Introduce FunctionTimer context manager
    • Wraps a function and times its execution duration
    • Has a get_data method that allows retrieval of timing data

Review Requests

  • what do you think about using different clocks to get the start time and then measure the duration?

Risk assessment

Low risk, as this context manager is not used against any production code.

@DerekMaggio DerekMaggio changed the base branch from edge to EXEC-372-hide-performance-metrics-project-behind-ff April 8, 2024 16:41
@DerekMaggio DerekMaggio force-pushed the EXEC-366_function_timer branch 2 times, most recently from 8cddde2 to f5a0ac5 Compare April 8, 2024 16:43
Copy link
Member

@sfoster1 sfoster1 left a comment

Choose a reason for hiding this comment

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

Before you go too hard into this, definitely take a look through

Function level time tracing can absolutely wreck performance to a human-noticeable level if done incorrectly, and "correctly" means integration at the lowest possible level and the tightest possible system interaction. I hope we can use some of the builtin stuff, but I've also marked up the PR contents basically as what we should do if we can't use what's built in.

"""Set the end time of the function."""
self._end_time = self._get_datetime()

def _get_datetime(self) -> datetime:
Copy link
Member

Choose a reason for hiding this comment

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

datetime is not the right interface for this. datetime is an interface focused on "correctly expressing points in time as understood by humans", and even its duration measures are about fundamentally the difference between those human points in time. That makes it far too slow for performance tracking at a low level. The thing that we're going to do here is use the lowest level possible interface, the thing with the least cover over something like clock_gettime with CLOCK_MONOTONIC. In python that's probably time.clock_gettime_ns and reporting only, as far as we can get away with it, durations.

Copy link
Contributor Author

@DerekMaggio DerekMaggio Apr 8, 2024

Choose a reason for hiding this comment

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

time.perf_counter_ns provides the highest resolution with the lowest overhead, so I will switch to that. The profile and trace library add a lot of overhead without adding any more needed utility.

end_time: datetime


class FunctionTimer(
Copy link
Member

Choose a reason for hiding this comment

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

This almost certainly wants to be a decorator rather than a context manager. The reason for that is that a context manager must be entered and exited around each call to the instrumented function as well as doing the actual timing part.

A decorator will run just at file parse time and can replace the function under test with the minimum instrumented version: (1) grab time (2) forward the call (3) grab time (4) put the duration in some fast queue for later processing.

The decorator can even decide from some runtime flag - and since this is for the higher resolution profiling that doesn't need to be user settable, it can be an environment variable only - whether to instrument the function or not without paying any extra per-call cost for the logic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So I originally had this as inheriting from https://docs.python.org/3/library/contextlib.html#contextlib.ContextDecorator which allows for usage as both a decorator and context manager, for 2 reasons:

Reason 1
I originally wanted to be able to access the duration in the context of the currently executing code.
But that's incorrect because we don't care about duration during the execution of the underlying code.

Reason 2
I was thinking, maybe we wanted to wrap multiple lines of code. But if we do just pull it out into a function and decorate that.

Will switch over to just a decorator. Thank you!

@DerekMaggio DerekMaggio force-pushed the EXEC-366_function_timer branch from dc770bf to 93af4a8 Compare April 9, 2024 12:47
@DerekMaggio DerekMaggio changed the base branch from EXEC-372-hide-performance-metrics-project-behind-ff to edge April 9, 2024 12:51
@DerekMaggio DerekMaggio marked this pull request as ready for review April 9, 2024 13:13
@DerekMaggio DerekMaggio requested review from a team as code owners April 9, 2024 13:13
@DerekMaggio DerekMaggio changed the title Exec 366 function timer feat: function timer Apr 9, 2024
@DerekMaggio DerekMaggio changed the title feat: function timer feat(performance-metrics): function timer Apr 9, 2024
@DerekMaggio DerekMaggio marked this pull request as draft April 9, 2024 13:34
@DerekMaggio DerekMaggio marked this pull request as ready for review April 9, 2024 14:32
@DerekMaggio DerekMaggio force-pushed the EXEC-366_function_timer branch from c6d2598 to 640a3b4 Compare April 10, 2024 20:15
@DerekMaggio DerekMaggio changed the title feat(performance-metrics): function timer feat(performance-metrics): add FunctionTimer Apr 11, 2024
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