[MINOR] Clean up of isDebugEnabled() usages#2435
Conversation
| if(LOG.isDebugEnabled()) | ||
| LOG.debug("Begin Function "+fkey); | ||
|
|
||
| LOG.debug("Begin Function " + fkey); |
There was a problem hiding this comment.
The main difference here, is that the LOG.isDebugEnabled() is very cheap, while the string concatenation can be very expensive. Unfortunately to call the LOG.debug java has to resolve the string variable input, and that is the main time used, especially if Logging is disabled.
I do not know if there is a way to avoid the string concatenation, or somehow getting it delayed to after the logging is analysed to be off.
There was a problem hiding this comment.
I see your point, but isnt it in practice in micro-level noise for these simple concats? i think we might over-engineer here
Why are we sticking to apache commons logging? with the newer SLF4J we would could do stuff like this:
LOG.debug("Begin Function {}", fkey);also SLF4J might already be used inside apachae commons logging, since it's already in the pom
There was a problem hiding this comment.
This would be cool, but we should not change the logging framework, so we just need to verify.
940746b to
423df5c
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2435 +/- ##
============================================
+ Coverage 71.61% 71.65% +0.04%
- Complexity 49867 49892 +25
============================================
Files 1602 1603 +1
Lines 193054 193062 +8
Branches 37792 37743 -49
============================================
+ Hits 138247 138348 +101
+ Misses 44031 43999 -32
+ Partials 10776 10715 -61 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
cd23f73 to
d61285d
Compare
506b17b to
eaf69bb
Compare
Follow-up to apache#2432. Adds org.apache.sysds.utils.ParameterizedLogger, a commons-logging adapter with `{}` and `{%fmt}` placeholders that absorbs the isEnabled() guard, so `if(LOG.isDebugEnabled()) LOG.debug( String.format(...))` collapses to `LOG.debug("... {%fmt}", arg)`. Refactors call sites and adds a unit test covering format() branches and dispatch.
eaf69bb to
fba5fc4
Compare
This PR is a followup to #2432 .
We have 131 Usages of LOG.isDebugEnabled()
This PR adds a new utils for Parameterized Logging
This helps with readability and simplifies code coverage