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

Update SPerformanceTimer to log immediate #1823

Closed
wants to merge 1 commit into from

Conversation

marcaaron
Copy link
Contributor

cc @coleaeason curious what you will say about these changes or if you have ideas on how else to do this. We appear to be using SPerformanceTimer in only one place. So, I decided to make it a general purpose / single use timer by default.

Details

Related to some logging I'd like to do in slow commands for fast-apis

Fixed Issues

Related to https://github.com/Expensify/Expensify/issues/393215

Tests


Internal Testing Reminder: when changing bedrock, please compile auth against your new changes

@marcaaron marcaaron self-assigned this Jul 25, 2024
@coleaeason
Copy link
Contributor

Adding tyler for his thoughts on this vs something else

if (_logImmediate) {
double durationMS = chrono::duration_cast<chrono::duration<double, std::milli>>(duration).count();
SINFO(_description << " " << _lastType << " in " << durationMS << " ms.");
return;
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems to break the purpose of this class, as we no longer record the totals. Either that or it makes this into a fairly complex way to do a simpler job.

I am confused what the purpose of this change is. I'm guessing you want to use this class somewhere new, but for a simpler task.

It seems we could do that with a class that looks like:

class NewClass {
    chrono::steady_clock::time_point _lastStart = 0;
    void start() {
        _lastStart = 0;
    }
    void stop() {
        SINFO("Message about" << now - _lastStart);
    }
};

I wonder if we can collapse SAutoTimer and SPerformanceTimer into one thing, since they seem to do basically the same task, and then make a simpler timer class oif that's what we need.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. I don't have super strong feelings on this so let me know whatever you'd prefer. I am mainly looking for something to time some parts of the Auth code. I originally proposed to add a Bench class to Auth, but @coleaeason thought maybe we should use something that already exists.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe add the Bench class in auth and create an issue to collapse these two Bedrock classes? It sounds like bench needs less functionality than we have here, and we can probably use only one of the two existing classes in bedrock.

@marcaaron marcaaron closed this Aug 21, 2024
@marcaaron marcaaron deleted the marcaaron-SPerformanceTimer branch August 21, 2024 00:44
@marcaaron
Copy link
Contributor Author

Created an issue to look into consolidating the timers, but does not seem like a priority right now so closing this.

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.

3 participants