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 StaticString for Parameter file and function #136

Closed
ShivaHuang opened this issue Jun 10, 2020 · 5 comments
Closed

Use StaticString for Parameter file and function #136

ShivaHuang opened this issue Jun 10, 2020 · 5 comments
Labels
kind/support Adopter support requests.

Comments

@ShivaHuang
Copy link
Contributor

ShivaHuang commented Jun 10, 2020

Abstract

The proposal from SSWG: Server Logging API suggest using String for parameters like #file and #function.

func log(level: Logger.Level,
             message: Logger.Message,
             metadata: Logger.Metadata?,
             file: String, function: String, line: UInt)

But is using StaticString a better design in practice?

Current Situation

Log functions take String for #file and #function. But some logging frameworks (backends) take StaticString for these parameters.

Problem

The main problem is String can not convert to StaticString, so for those frameworks take StaticString as parameters, there is no way to build a LogHandler for them.

The only workaround I found is discard these information. For those backends don't care about the #file or #function this will be fine, like swift-log-oslog.

However, the others will always get the #file and #function in LogHandler, not the actual place in the program. This will be a big problem.

Proposed Solution

If we changed the parameter type from String to StaticString, those backends require StaticString can take it without problems. And those require String can easily convert it to what they want. A simple extension can do the trick.

extension StaticString {
    @inlinable
    var stringValue: String {
        self.withUTF8Buffer { String(decoding: $0, as: UTF8.self) }
    }
}

The cons of this change is, it breaks the API compatibility, log handler providers need to update their code for this chage.

Any suggestions about this idea?

@ktoso
Copy link
Member

ktoso commented Jun 10, 2020

I believe @weissi explored this topic in great depth with specific focus on OSLog so he should be able to answer best here.

@ktoso ktoso added the kind/support Adopter support requests. label Jun 10, 2020
@weissi
Copy link
Member

weissi commented Jun 15, 2020

I think String is better than StaticString, it should in fact be faster.

  • String is exactly 2 words wide, StaticString is 2 words + 1 byte wide (I know, a mistake that's now unfixable with ABI). So passing a StaticString knocks out 3 registers, String only 2
  • String is more powerful than StaticString
  • String also has an "immortal" representation. So if you create a String that is in fact a static string, it won't allocate and its pointer will go straight into the data section.

@ShivaHuang I would suggest that everybody switch to String instead of StaticString. But do you have a concrete logging backend in mind where you'd need to pass file as a StaticString?

@ShivaHuang
Copy link
Contributor Author

Great point!

I misunderstood the performance and memory usage between String and StaticString. And I agree that String is overall more suitable than StaticString.

The logging backend use StaticString I encounter is CocoaLumberjack.

However, after digging into their code, they also convert the StaticString to String before passing it to the underlay OC code. here
I think I should submit a PR to CocoaLumberjack for that. 😉

BTW, while searching some similar use case in Apple's SDK, I found some functions use StaticString for the same propose. like

  • XCTAssertEqual from XCTest
  • assertionFailure(_:file:line:) from Swift Standard Library

Will Apple also change their code? 🤣

@weissi
Copy link
Member

weissi commented Jun 19, 2020

@ShivaHuang until Swift 4.2, StaticString was much faster which is why XCTAssertEqual was designed the way it is. Same for assertionFailure etc, now they can’t easily change that because of source/ABI compat.

Also: StaticString should just be 1 word, there’s no reason except that nobody noticed until the ABI was fixed 😬

@ShivaHuang
Copy link
Contributor Author

I'll close this issue.

Thank you @ktoso and @weissi, I learned a lot in this discussion.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/support Adopter support requests.
Projects
None yet
Development

No branches or pull requests

3 participants