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

using dateFormatter is really inefficient #80

Closed
nagypeterjob opened this issue Mar 4, 2023 · 3 comments
Closed

using dateFormatter is really inefficient #80

nagypeterjob opened this issue Mar 4, 2023 · 3 comments

Comments

@nagypeterjob
Copy link
Contributor

nagypeterjob commented Mar 4, 2023

Hey @sushichop 👋🏻

First of all, thank you for working on Puppy! Puppy is my go-to Swift logging lib, I really appreciate the amount of work you are investing in it.

Now to the issue. There is a section in the README, which shows how to use LogFormattable. In this example, the date value is returned by the dateFormatter function:

struct LogFormatter: LogFormattable {
    func formatMessage(_ level: LogLevel, message: String, tag: String, function: String,
                       file: String, line: UInt, swiftLogInfo: [String : String],
                       label: String, date: Date, threadID: UInt64) -> String {
        let date = dateFormatter(date)
        let fileName = fileName(file)
        let moduleName = moduleName(file)
        return "\(date) \(threadID) [\(level.emoji) \(level)] \(swiftLogInfo) \(moduleName)/\(fileName)#L.\(line) \(function) \(message)".colorize(level.color)
    }
}

dateFormatter looks like this:

@Sendable
public func dateFormatter(_ date: Date, locale: String = "en_US_POSIX", dateFormat: String = "yyyy-MM-dd'T'HH:mm:ss.SSSZZZZZ", timeZone: String = "") -> String {
    let dateFormatter = DateFormatter()
    dateFormatter.locale = Locale(identifier: locale)
    dateFormatter.dateFormat = dateFormat
    dateFormatter.timeZone = TimeZone(identifier: timeZone)
    return dateFormatter.string(from: date)
}

You see, calling the DateFormatter() constructor is a really expensive operation, and in this case, it is being called on each log event. Performance can be greatly improved by pre-initialising the DateFormatter() object, and injecting it into the formatMessage function.

There are multiple ways to address this, would you like me to create a PR?

@sushichop
Copy link
Owner

@nagypeterjob
Thanks for sharing your idea!
Yes. Every PR is always welcome🙂

@nagypeterjob
Copy link
Contributor Author

I have pushed a PR, let me know what you think @sushichop

@sushichop
Copy link
Owner

@nagypeterjob
Thanks for your PR. I have merged your PR.

It makes sense not to call DateFormatter() on each log event.
In addition, it's cool that the performance measurement has been added in tests👍

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

No branches or pull requests

2 participants