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

Inline log function with message lambda #1271

Closed
wants to merge 1 commit into from

Conversation

octa-one
Copy link
Contributor

Minor improvement.
When calling log(level: Level, msg: () -> MESSAGE) function with an inline modifier, no lambda object is created. This will slightly improve performance.
I also removed canLog(...) function, because it is completely similar to the isAt(...) function.

@arnaudgiuliani
Copy link
Member

what about perf impact? I'm always worried about impact as such level

@octa-one
Copy link
Contributor Author

Sorry for late reply. What do you mean about performance impact?
If we add an inline modifier, as I suggest, Kotlin compiler will place the condition if (isAt(level)) { } from the function body at the call site, and will not create an extra instance for the lambda in function arguments.

Less object will be created, which will have a positive effect on performance, especially considering that logging is called quite often.

@arnaudgiuliani arnaudgiuliani added this to the 3.2.0 milestone Mar 22, 2022
@arnaudgiuliani
Copy link
Member

Need to check in our latest version of Kotlin, but string evaluation could be done even if the string is in a lambda: like log() { "a message here" }. The message could be even evaluated. This is what we don't want, as the logs that aren't already inlined (because there's is already an API for that) mustn't evaluate messages that could provide heavier information load.

@octa-one
Copy link
Contributor Author

octa-one commented May 7, 2022

Didn’t change anything, forgot that I already have 2 branches with pull requests.

@arnaudgiuliani arnaudgiuliani modified the milestones: 3.3.0, 3.2.1 May 12, 2022
@arnaudgiuliani arnaudgiuliani added the status:accepted accepted to be developed label May 12, 2022
@arnaudgiuliani arnaudgiuliani modified the milestones: 3.2.1, 3.3.0 Jun 27, 2022
@ANIKINKIRILL
Copy link

why this PR is still not merged? what does stop this?

@arnaudgiuliani
Copy link
Member

It's planned for 3.3.0 as part of new internal. Then, not released for 3.2.1 where we bring fixes only & library patch updates.

@arnaudgiuliani
Copy link
Member

Log API has been revamped https://github.com/InsertKoinIO/koin/blob/core/3.3.0/core/koin-core/src/commonMain/kotlin/org/koin/core/logger/Logger.kt

Thanks for your proposal. Feel free to reopen discussion on new API

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants