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

Option to filter logs by priority #348

Merged
merged 6 commits into from Jan 31, 2022
Merged

Conversation

philips77
Copy link
Member

This PR fixes #346.
The new method

/**
* Returns the minimum log priority that should be logged.
* @return Minimum log priority.
* @since 2.4.0
*/
@LogPriority int getMinLogPriority();

in BleManager and BleServerManager allows to omit creating and logging logs on lower priorities. By default, logs from levels Log.INFO and higher will be logged, which is a change in default behavior. Previously all logs were produced, even if later ignored.
To disable logging, return Log.ERROR or Log.ASSERT from those methods. To restore previous behavior, return Log.VERBOSE.

@x2764tech
Copy link

Thanks for all this work @philips77

Have you memory profiled this?

My fear is that, although this allows log filtering, it doesn't necessarily prevent the issue of object allocation.


My understanding of how lambdas work in Java is that each lambda potentially allocates a new object that captures the closure information.
So, log(Log.INFO, () -> "Hello") is roughly equivalent to log(Log.INFO, new Loggable() { String log() { return "Hello"; } }).
Which means an object allocation even when the log level is disabled.
Of course, the runtime implementation may be able in inline some or all of this, but this has the potential to vary from phone to phone, and android version to android version.

Unfortunately, the only way I've successfully implemented memory efficient log filtering is by using if statements at the site of logging.
so log(Log.INFO, "Hello " + state) becomes if(isLogLevelEnabled(Log.INFO)) { log(Log.INFO, "Hello " + state) }.
It's tempting to use language features to eliminate this, as you have done, but, as I've shown above, this doesn't really solve the problem of object allocation when a log level isn't enabled.

@philips77
Copy link
Member Author

I precisely wanted to

to use language features to eliminate this

as it was tempting. However, it's still way faster than before. I ca refactor it once again to add tons of ifs :(

This change improves performance, as with disabled logs the byte arrays are not converted to String just for logging purposes.
@philips77 philips77 force-pushed the improvement/disabling-logging branch from a169581 to d361279 Compare January 27, 2022 13:53
@x2764tech
Copy link

@philips77 are you able to memory profile this before doing all that work?
On Android 11, I'm seeing lots of memory churn because of the allocations for Strings for logging in notifications.

@philips77
Copy link
Member Author

Hello,

Allocating objects for lambdas should still be much better than the previous solution. I'll leave the current implementation for now, as we're thinking about redoing this project in Kotlin anyway. I'll add some more features, check reported issues and will release it as 2.4.

@philips77 philips77 merged commit a10717a into master Jan 31, 2022
@philips77 philips77 deleted the improvement/disabling-logging branch January 31, 2022 14:31
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.

Memory allocation due to logging
2 participants