You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@logging.apache.org by GitBox <gi...@apache.org> on 2022/09/26 07:10:46 UTC
[GitHub] [logging-log4cxx] swebb2066 opened a new pull request, #135: Reduce disabled logger overhead
swebb2066 opened a new pull request, #135:
URL: https://github.com/apache/logging-log4cxx/pull/135
Prior to the PR, next_stable testing disabled logger performance was 20% worse than master (13.0).
By adding a cached level to Logger, testing disabled logger performance improves on master by 50%.
Automatic repository configuration is now explicit part (autoConfigure() function) of the LoggerRepository interface, and LogManager is responsible for calling autoConfigure(). This is required to have correct state for the cached level thresholds.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4cxx] rm5248 commented on pull request #135: Reduce disabled logger overhead
Posted by GitBox <gi...@apache.org>.
rm5248 commented on PR #135:
URL: https://github.com/apache/logging-log4cxx/pull/135#issuecomment-1259370177
I'll need to test this out sometime in the next few days, but that looks good to me. One thing that I would like to double-check is to make sure that re-configuring works correctly at runtime(e.g. calling DOMConfigurator::configure), since it looks like the reparenting and level checking has changed slightly.
Are there any other commonly accessed fields that may need to be private? In this case adding it as a private member variable instead of in the private class is reasonable to do for performance purposes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4cxx] rm5248 commented on pull request #135: Reduce disabled logger overhead
Posted by GitBox <gi...@apache.org>.
rm5248 commented on PR #135:
URL: https://github.com/apache/logging-log4cxx/pull/135#issuecomment-1262992462
So after taking a look at this, what I was afraid of(not propagating the level to sub-loggers) is true. For example, if we have loggers `com` and `com.example` and we reconfigure to set the logger `com` to be `TRACE` for example, the `com.example` logger does not also get this level. This is assuming that the log configuration is coming from an XML file, although I think this would also be the case if you manually set the log levels. If you load an XML file with `com` set to `TRACE` at the beginning of execution, you will see the `com` and `com.example` messages output.
This currently works fine in next_stable. I'll add a test for this feature soon, since it is something that I depend on often. There is a related issue, if you remove a logger from the configuration file and reload the file the logger will retain its currently set level.
Unfortunately I don't really see an easy way of fixing this. We could reconfigure everything once we load the file, but that leads to a less than ideal situation of if a user is manually setting the log level they would now have to do an extra call at the end to have all of the loggers cache their new threshold level.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4cxx] rm5248 commented on a diff in pull request #135: Reduce disabled logger overhead
Posted by GitBox <gi...@apache.org>.
rm5248 commented on code in PR #135:
URL: https://github.com/apache/logging-log4cxx/pull/135#discussion_r985089471
##########
src/main/cpp/hierarchy.cpp:
##########
@@ -433,6 +433,22 @@ void Hierarchy::updateChildren(ProvisionNode& pn, const LoggerPtr& logger)
l->setParent( logger );
}
}
+
+}
+
+void Hierarchy::updateChildren(const Logger* parent)
+{
+ for (auto& item : m_priv->loggers)
+ {
+ for (auto l = item.second; l; l = l->getParent())
+ {
+ if (l->getParent().get() == parent)
Review Comment:
Oh yes you're right, I misread what the code was doing.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4cxx] rm5248 commented on a diff in pull request #135: Reduce disabled logger overhead
Posted by GitBox <gi...@apache.org>.
rm5248 commented on code in PR #135:
URL: https://github.com/apache/logging-log4cxx/pull/135#discussion_r984568574
##########
src/main/cpp/hierarchy.cpp:
##########
@@ -433,6 +433,22 @@ void Hierarchy::updateChildren(ProvisionNode& pn, const LoggerPtr& logger)
l->setParent( logger );
}
}
+
+}
+
+void Hierarchy::updateChildren(const Logger* parent)
+{
+ for (auto& item : m_priv->loggers)
+ {
+ for (auto l = item.second; l; l = l->getParent())
+ {
+ if (l->getParent().get() == parent)
Review Comment:
I'm pretty sure this is going to only go one level deep though? so `com.example` would get set but not `com.example.foo`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4cxx] swebb2066 commented on a diff in pull request #135: Reduce disabled logger overhead
Posted by GitBox <gi...@apache.org>.
swebb2066 commented on code in PR #135:
URL: https://github.com/apache/logging-log4cxx/pull/135#discussion_r984899172
##########
src/main/cpp/hierarchy.cpp:
##########
@@ -433,6 +433,22 @@ void Hierarchy::updateChildren(ProvisionNode& pn, const LoggerPtr& logger)
l->setParent( logger );
}
}
+
+}
+
+void Hierarchy::updateChildren(const Logger* parent)
+{
+ for (auto& item : m_priv->loggers)
+ {
+ for (auto l = item.second; l; l = l->getParent())
+ {
+ if (l->getParent().get() == parent)
Review Comment:
`for (auto l = item.second; l; l = l->getParent())` goes through each parent until root. A `getRootLoggger().setLevel()` will find that `com.example.foo` and `com.example` has 'com' in the parent hierarchy.
`for (auto& item : m_priv->loggers)` checks every configured logger in the application.
I will add a test to confirm.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4cxx] swebb2066 commented on pull request #135: Reduce disabled logger overhead
Posted by GitBox <gi...@apache.org>.
swebb2066 commented on PR #135:
URL: https://github.com/apache/logging-log4cxx/pull/135#issuecomment-1264541240
**Are there any other commonly accessed fields that may need to be private?
Not that I can see,
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4cxx] swebb2066 commented on pull request #135: Reduce disabled logger overhead
Posted by GitBox <gi...@apache.org>.
swebb2066 commented on PR #135:
URL: https://github.com/apache/logging-log4cxx/pull/135#issuecomment-1263136206
** Unfortunately I don't really see an easy way of fixing this.
I think the logger can ask the Hierarchy to update its children - Hierachy has all the loggers.
The principle here is that work should br done in infrequent events (e.g. changing a level) in preference to burdening every frequent event.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [logging-log4cxx] swebb2066 commented on a diff in pull request #135: Reduce disabled logger overhead
Posted by GitBox <gi...@apache.org>.
swebb2066 commented on code in PR #135:
URL: https://github.com/apache/logging-log4cxx/pull/135#discussion_r984899172
##########
src/main/cpp/hierarchy.cpp:
##########
@@ -433,6 +433,22 @@ void Hierarchy::updateChildren(ProvisionNode& pn, const LoggerPtr& logger)
l->setParent( logger );
}
}
+
+}
+
+void Hierarchy::updateChildren(const Logger* parent)
+{
+ for (auto& item : m_priv->loggers)
+ {
+ for (auto l = item.second; l; l = l->getParent())
+ {
+ if (l->getParent().get() == parent)
Review Comment:
`for (auto l = item.second; l; l = l->getParent())` goes through each parent until root. A `getRootLoggger().setLevel()` will find that `com.example.foo` and `com.example` has 'com' in the parent hierarchy.
`for (auto& item : m_priv->loggers)` checks every configured logger in the hierarchy.
I will add a test to confirm.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org