You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sling.apache.org by GitBox <gi...@apache.org> on 2021/07/06 19:20:48 UTC
[GitHub] [sling-org-apache-sling-scripting-core] joerghoh opened a new pull request #8: SLING-10584 log an info message if adding bindings takes more than 1 msec
joerghoh opened a new pull request #8:
URL: https://github.com/apache/sling-org-apache-sling-scripting-core/pull/8
--
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: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-scripting-core] kwin edited a comment on pull request #8: SLING-10584 log an info message if adding bindings takes more than 1 msec
Posted by GitBox <gi...@apache.org>.
kwin edited a comment on pull request #8:
URL: https://github.com/apache/sling-org-apache-sling-scripting-core/pull/8#issuecomment-875446143
I think this deserves a more general discussion about which SPI calls bound to a request should be measured and lead to a warning in case they take too long. IMHO there are dozens of places with which custom code can make request execution much slower. I don't see a particular need to just measure execution of `BindingsValuesProvider` and emit warnings. What about servlet filters/JSP tags/HTL plugins/getter methods of beans/.....?
Also for special bindings providers it may be totally fine if they take more than 1ms (if the bindings are used rarely). Then this output would just pollute the log.
--
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: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-scripting-core] joerghoh commented on pull request #8: SLING-10584 log an info message if adding bindings takes more than 1 msec
Posted by GitBox <gi...@apache.org>.
joerghoh commented on pull request #8:
URL: https://github.com/apache/sling-org-apache-sling-scripting-core/pull/8#issuecomment-875333380
``System.nanoTime()`` is used heavily within the sling engine (the microseconds in the ``Recent Requests`` are calculated based on this, see [here](https://github.com/apache/sling-org-apache-sling-engine/blob/master/src/main/java/org/apache/sling/engine/impl/request/SlingRequestProgressTracker.java)).
I don't think that on current versions of OSes it is an expensive operation, which should be avoided.
--
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: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-scripting-core] sonarcloud[bot] commented on pull request #8: SLING-10584 log an info message if adding bindings takes more than 1 msec
Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] commented on pull request #8:
URL: https://github.com/apache/sling-org-apache-sling-scripting-core/pull/8#issuecomment-875025816
Kudos, SonarCloud Quality Gate passed!
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/bug.png' alt='Bug' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-core&pullRequest=8&resolved=false&types=BUG) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-core&pullRequest=8&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-core&pullRequest=8&resolved=false&types=BUG)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/vulnerability.png' alt='Vulnerability' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-core&pullRequest=8&resolved=false&types=VULNERABILITY) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-core&pullRequest=8&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-core&pullRequest=8&resolved=false&types=VULNERABILITY)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/security_hotspot.png' alt='Security Hotspot' width='16' height='16' />](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-scripting-core&pullRequest=8&resolved=false&types=SECURITY_HOTSPOT) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-scripting-core&pullRequest=8&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-scripting-core&pullRequest=8&resolved=false&types=SECURITY_HOTSPOT)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/code_smell.png' alt='Code Smell' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-core&pullRequest=8&resolved=false&types=CODE_SMELL) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-core&pullRequest=8&resolved=false&types=CODE_SMELL) [0 Code Smells](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-core&pullRequest=8&resolved=false&types=CODE_SMELL)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/CoverageChart/25.png' alt='33.3%' width='16' height='16' />](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-scripting-core&pullRequest=8&metric=new_coverage&view=list) [33.3% Coverage](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-scripting-core&pullRequest=8&metric=new_coverage&view=list)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/Duplications/3.png' alt='0.0%' width='16' height='16' />](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-scripting-core&pullRequest=8&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-scripting-core&pullRequest=8&metric=new_duplicated_lines_density&view=list)
--
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: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-scripting-core] bdelacretaz commented on a change in pull request #8: SLING-10584 log an info message if adding bindings takes more than 1 msec
Posted by GitBox <gi...@apache.org>.
bdelacretaz commented on a change in pull request #8:
URL: https://github.com/apache/sling-org-apache-sling-scripting-core/pull/8#discussion_r665118859
##########
File path: src/main/java/org/apache/sling/scripting/core/impl/DefaultSlingScript.java
##########
@@ -732,7 +732,14 @@ Bindings verifySlingBindings(final SlingBindings slingBindings) throws IOExcepti
ProtectedBindings protectedBindings = new ProtectedBindings(bindings, protectedKeys);
for (BindingsValuesProvider provider : bindingsValuesProviders) {
+ long start = System.nanoTime();
provider.addBindings(protectedBindings);
+ long stop = System.nanoTime();
+ if (stop-start > (1000*1000)) { // 1 ms
Review comment:
it would be good to use a named constant for this value, also to avoid duplicating it in the ScriptContextProvider
--
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: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-scripting-core] bdelacretaz commented on a change in pull request #8: SLING-10584 log an info message if adding bindings takes more than 1 msec
Posted by GitBox <gi...@apache.org>.
bdelacretaz commented on a change in pull request #8:
URL: https://github.com/apache/sling-org-apache-sling-scripting-core/pull/8#discussion_r681561784
##########
File path: src/main/java/org/apache/sling/scripting/core/impl/bundled/ScriptContextProvider.java
##########
@@ -105,7 +110,16 @@ public ExecutableContext prepareScriptContext(SlingHttpServletRequest request, S
ProtectedBindings protectedBindings = new ProtectedBindings(bindings, PROTECTED_BINDINGS);
for (BindingsValuesProvider bindingsValuesProvider : bvpTracker.getBindingsValuesProviders(scriptEngine.getFactory(),
BindingsValuesProvider.DEFAULT_CONTEXT)) {
+ long start = System.nanoTime();
bindingsValuesProvider.addBindings(protectedBindings);
+ long stop = System.nanoTime();
+ LOG.trace("Invoking addBindings() of {} took {} nanoseconds",
+ bindingsValuesProvider.getClass().getName(), stop-start);
+ if ((stop-start) > WARN_LIMIT_FOR_BVP_NANOS) {
+ LOG.info("Adding the bindings of {} took {} microseconds;"
Review comment:
same nitpick as above, "which is above..."
--
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: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-scripting-core] sonarcloud[bot] removed a comment on pull request #8: SLING-10584 log an info message if adding bindings takes more than 1 msec
Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] removed a comment on pull request #8:
URL: https://github.com/apache/sling-org-apache-sling-scripting-core/pull/8#issuecomment-875370387
Kudos, SonarCloud Quality Gate passed!
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/bug.png' alt='Bug' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-core&pullRequest=8&resolved=false&types=BUG) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-core&pullRequest=8&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-core&pullRequest=8&resolved=false&types=BUG)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/vulnerability.png' alt='Vulnerability' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-core&pullRequest=8&resolved=false&types=VULNERABILITY) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-core&pullRequest=8&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-core&pullRequest=8&resolved=false&types=VULNERABILITY)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/security_hotspot.png' alt='Security Hotspot' width='16' height='16' />](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-scripting-core&pullRequest=8&resolved=false&types=SECURITY_HOTSPOT) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-scripting-core&pullRequest=8&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-scripting-core&pullRequest=8&resolved=false&types=SECURITY_HOTSPOT)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/code_smell.png' alt='Code Smell' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-core&pullRequest=8&resolved=false&types=CODE_SMELL) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-core&pullRequest=8&resolved=false&types=CODE_SMELL) [0 Code Smells](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-core&pullRequest=8&resolved=false&types=CODE_SMELL)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/CoverageChart/50.png' alt='53.3%' width='16' height='16' />](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-scripting-core&pullRequest=8&metric=new_coverage&view=list) [53.3% Coverage](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-scripting-core&pullRequest=8&metric=new_coverage&view=list)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/Duplications/3.png' alt='0.0%' width='16' height='16' />](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-scripting-core&pullRequest=8&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-scripting-core&pullRequest=8&metric=new_duplicated_lines_density&view=list)
--
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: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-scripting-core] kwin commented on pull request #8: SLING-10584 log an info message if adding bindings takes more than 1 msec
Posted by GitBox <gi...@apache.org>.
kwin commented on pull request #8:
URL: https://github.com/apache/sling-org-apache-sling-scripting-core/pull/8#issuecomment-875032568
I think using `System.nanoTime` might be too expensive to use unconditionally (https://stackoverflow.com/a/19052481)
--
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: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-scripting-core] joerghoh merged pull request #8: SLING-10584 log an info message if adding bindings takes more than 1 msec
Posted by GitBox <gi...@apache.org>.
joerghoh merged pull request #8:
URL: https://github.com/apache/sling-org-apache-sling-scripting-core/pull/8
--
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: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-scripting-core] kwin commented on pull request #8: SLING-10584 log an info message if adding bindings takes more than 1 msec
Posted by GitBox <gi...@apache.org>.
kwin commented on pull request #8:
URL: https://github.com/apache/sling-org-apache-sling-scripting-core/pull/8#issuecomment-875446143
I think this deserves a more general discussion about which SPI calls bound to a request should be measured and lead to a warning in case they take too long. IMHO there are dozens of places with which custom code can make request execution much slower. I don't see a particular need to just measure execution of `BindingsValuesProvider` and emit warnings. What about servlet filters/JSP tags/HTL plugins/getter methods of beans/.....?
--
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: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-scripting-core] bdelacretaz commented on pull request #8: SLING-10584 log an info message if adding bindings takes more than 1 msec
Posted by GitBox <gi...@apache.org>.
bdelacretaz commented on pull request #8:
URL: https://github.com/apache/sling-org-apache-sling-scripting-core/pull/8#issuecomment-875373107
> ...true about the duplication, but I haven't found a good way to avoid it at all or where to put it...
If there's no good place for that, I suggest a named constant in each class with a comment that mentions the duplication:
// Note that this constant is duplicated in several source files
public static final int WARN_IF_LONGER_THAN_NSEC = 1000 * 1000;
My failure case is changing it one place and forgetting the others, which this avoids.
> ...measuring the timing of some operation is a concern we have in many places, and for which we don't have a good overall solution...
Agreed, there's probably a library or pattern for that out there...
--
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: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-scripting-core] joerghoh commented on pull request #8: SLING-10584 log an info message if adding bindings takes more than 1 msec
Posted by GitBox <gi...@apache.org>.
joerghoh commented on pull request #8:
URL: https://github.com/apache/sling-org-apache-sling-scripting-core/pull/8#issuecomment-875368666
@bdelacretaz true about the duplication, but I haven't found a good way to avoid it at all or where to put it. But also measuring the timing of some operation is a concern we have in many places, and for which we don't have a good overall solution.
--
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: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-scripting-core] sonarcloud[bot] commented on pull request #8: SLING-10584 log an info message if adding bindings takes more than 1 msec
Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] commented on pull request #8:
URL: https://github.com/apache/sling-org-apache-sling-scripting-core/pull/8#issuecomment-875370387
Kudos, SonarCloud Quality Gate passed!
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/bug.png' alt='Bug' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-core&pullRequest=8&resolved=false&types=BUG) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-core&pullRequest=8&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-core&pullRequest=8&resolved=false&types=BUG)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/vulnerability.png' alt='Vulnerability' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-core&pullRequest=8&resolved=false&types=VULNERABILITY) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-core&pullRequest=8&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-core&pullRequest=8&resolved=false&types=VULNERABILITY)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/security_hotspot.png' alt='Security Hotspot' width='16' height='16' />](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-scripting-core&pullRequest=8&resolved=false&types=SECURITY_HOTSPOT) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-scripting-core&pullRequest=8&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-scripting-core&pullRequest=8&resolved=false&types=SECURITY_HOTSPOT)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/code_smell.png' alt='Code Smell' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-core&pullRequest=8&resolved=false&types=CODE_SMELL) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-core&pullRequest=8&resolved=false&types=CODE_SMELL) [0 Code Smells](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-core&pullRequest=8&resolved=false&types=CODE_SMELL)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/CoverageChart/50.png' alt='53.3%' width='16' height='16' />](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-scripting-core&pullRequest=8&metric=new_coverage&view=list) [53.3% Coverage](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-scripting-core&pullRequest=8&metric=new_coverage&view=list)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/Duplications/3.png' alt='0.0%' width='16' height='16' />](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-scripting-core&pullRequest=8&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-scripting-core&pullRequest=8&metric=new_duplicated_lines_density&view=list)
--
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: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-scripting-core] sonarcloud[bot] commented on pull request #8: SLING-10584 log an info message if adding bindings takes more than 1 msec
Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] commented on pull request #8:
URL: https://github.com/apache/sling-org-apache-sling-scripting-core/pull/8#issuecomment-875359721
Kudos, SonarCloud Quality Gate passed!
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/bug.png' alt='Bug' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-core&pullRequest=8&resolved=false&types=BUG) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-core&pullRequest=8&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-core&pullRequest=8&resolved=false&types=BUG)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/vulnerability.png' alt='Vulnerability' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-core&pullRequest=8&resolved=false&types=VULNERABILITY) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-core&pullRequest=8&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-core&pullRequest=8&resolved=false&types=VULNERABILITY)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/security_hotspot.png' alt='Security Hotspot' width='16' height='16' />](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-scripting-core&pullRequest=8&resolved=false&types=SECURITY_HOTSPOT) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-scripting-core&pullRequest=8&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-scripting-core&pullRequest=8&resolved=false&types=SECURITY_HOTSPOT)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/code_smell.png' alt='Code Smell' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-core&pullRequest=8&resolved=false&types=CODE_SMELL) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-core&pullRequest=8&resolved=false&types=CODE_SMELL) [0 Code Smells](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-core&pullRequest=8&resolved=false&types=CODE_SMELL)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/CoverageChart/50.png' alt='53.3%' width='16' height='16' />](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-scripting-core&pullRequest=8&metric=new_coverage&view=list) [53.3% Coverage](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-scripting-core&pullRequest=8&metric=new_coverage&view=list)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/Duplications/3.png' alt='0.0%' width='16' height='16' />](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-scripting-core&pullRequest=8&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-scripting-core&pullRequest=8&metric=new_duplicated_lines_density&view=list)
--
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: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-scripting-core] bdelacretaz commented on a change in pull request #8: SLING-10584 log an info message if adding bindings takes more than 1 msec
Posted by GitBox <gi...@apache.org>.
bdelacretaz commented on a change in pull request #8:
URL: https://github.com/apache/sling-org-apache-sling-scripting-core/pull/8#discussion_r681561449
##########
File path: src/main/java/org/apache/sling/scripting/core/impl/DefaultSlingScript.java
##########
@@ -732,7 +735,16 @@ Bindings verifySlingBindings(final SlingBindings slingBindings) throws IOExcepti
ProtectedBindings protectedBindings = new ProtectedBindings(bindings, protectedKeys);
for (BindingsValuesProvider provider : bindingsValuesProviders) {
+ long start = System.nanoTime();
provider.addBindings(protectedBindings);
+ long stop = System.nanoTime();
+ LOGGER.trace("Invoking addBindings() of {} took {} nanoseconds",
+ provider.getClass().getName(), stop-start);
+ if (stop-start > WARN_LIMIT_FOR_BVP_NANOS) {
+ LOGGER.info("Adding the bindings of {} took {} microseconds; "
Review comment:
nitpick, this message could say "...which is above the built-in limit of $WARN_LIMIT_FOR_BVP_NANOS"
--
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: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-scripting-core] joerghoh commented on pull request #8: SLING-10584 log an info message if adding bindings takes more than 1 msec
Posted by GitBox <gi...@apache.org>.
joerghoh commented on pull request #8:
URL: https://github.com/apache/sling-org-apache-sling-scripting-core/pull/8#issuecomment-876369106
@kwin: The RequestProgressTracker already contains a ton of timing information which can help you to pinpoint performance problems in individual filters, JSPs or HTL snippets. Of course its capabilities are limited, not every functinoality writes timing information there.
I agree that we should have a more sophisticated framework which should allow us to get better timing information from relevant pieces of the stack without to instrument each and every functionality. But I don't see any trace of such a thing yet, not to speak of actual working code.
Regarding the noise in the logfiles created by this functionality. I tested it with AEM, and I found 4 warnings (for 4 distinct BVPs) once on startup, but that's it. The trace logging showed that the subsequent calls to ``addBindings()`` took 3-6 microseconds. So it's unlikely that the logs are filled with noise.
And the root cause of the problem is that the LayzBindings are not used consistently (which compute the variable only if used), but there are Bindings out there, which compute these values irrespective if they are used or not. Changing that code is problematic, if it's outside of your individual reach.
--
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: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-scripting-core] joerghoh commented on a change in pull request #8: SLING-10584 log an info message if adding bindings takes more than 1 msec
Posted by GitBox <gi...@apache.org>.
joerghoh commented on a change in pull request #8:
URL: https://github.com/apache/sling-org-apache-sling-scripting-core/pull/8#discussion_r682387841
##########
File path: src/main/java/org/apache/sling/scripting/core/impl/DefaultSlingScript.java
##########
@@ -732,7 +735,16 @@ Bindings verifySlingBindings(final SlingBindings slingBindings) throws IOExcepti
ProtectedBindings protectedBindings = new ProtectedBindings(bindings, protectedKeys);
for (BindingsValuesProvider provider : bindingsValuesProviders) {
+ long start = System.nanoTime();
provider.addBindings(protectedBindings);
+ long stop = System.nanoTime();
+ LOGGER.trace("Invoking addBindings() of {} took {} nanoseconds",
+ provider.getClass().getName(), stop-start);
+ if (stop-start > WARN_LIMIT_FOR_BVP_NANOS) {
+ LOGGER.info("Adding the bindings of {} took {} microseconds; "
Review comment:
implemented
--
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: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-scripting-core] sonarcloud[bot] removed a comment on pull request #8: SLING-10584 log an info message if adding bindings takes more than 1 msec
Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] removed a comment on pull request #8:
URL: https://github.com/apache/sling-org-apache-sling-scripting-core/pull/8#issuecomment-875359721
Kudos, SonarCloud Quality Gate passed!
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/bug.png' alt='Bug' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-core&pullRequest=8&resolved=false&types=BUG) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-core&pullRequest=8&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-core&pullRequest=8&resolved=false&types=BUG)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/vulnerability.png' alt='Vulnerability' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-core&pullRequest=8&resolved=false&types=VULNERABILITY) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-core&pullRequest=8&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-core&pullRequest=8&resolved=false&types=VULNERABILITY)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/security_hotspot.png' alt='Security Hotspot' width='16' height='16' />](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-scripting-core&pullRequest=8&resolved=false&types=SECURITY_HOTSPOT) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-scripting-core&pullRequest=8&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-scripting-core&pullRequest=8&resolved=false&types=SECURITY_HOTSPOT)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/code_smell.png' alt='Code Smell' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-core&pullRequest=8&resolved=false&types=CODE_SMELL) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-core&pullRequest=8&resolved=false&types=CODE_SMELL) [0 Code Smells](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-core&pullRequest=8&resolved=false&types=CODE_SMELL)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/CoverageChart/50.png' alt='53.3%' width='16' height='16' />](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-scripting-core&pullRequest=8&metric=new_coverage&view=list) [53.3% Coverage](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-scripting-core&pullRequest=8&metric=new_coverage&view=list)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/Duplications/3.png' alt='0.0%' width='16' height='16' />](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-scripting-core&pullRequest=8&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-scripting-core&pullRequest=8&metric=new_duplicated_lines_density&view=list)
--
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: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-scripting-core] sonarcloud[bot] removed a comment on pull request #8: SLING-10584 log an info message if adding bindings takes more than 1 msec
Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] removed a comment on pull request #8:
URL: https://github.com/apache/sling-org-apache-sling-scripting-core/pull/8#issuecomment-875025816
Kudos, SonarCloud Quality Gate passed!
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/bug.png' alt='Bug' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-core&pullRequest=8&resolved=false&types=BUG) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-core&pullRequest=8&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-core&pullRequest=8&resolved=false&types=BUG)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/vulnerability.png' alt='Vulnerability' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-core&pullRequest=8&resolved=false&types=VULNERABILITY) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-core&pullRequest=8&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-core&pullRequest=8&resolved=false&types=VULNERABILITY)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/security_hotspot.png' alt='Security Hotspot' width='16' height='16' />](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-scripting-core&pullRequest=8&resolved=false&types=SECURITY_HOTSPOT) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-scripting-core&pullRequest=8&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-scripting-core&pullRequest=8&resolved=false&types=SECURITY_HOTSPOT)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/common/code_smell.png' alt='Code Smell' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-core&pullRequest=8&resolved=false&types=CODE_SMELL) [<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/RatingBadge/A.png' alt='A' width='16' height='16' />](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-core&pullRequest=8&resolved=false&types=CODE_SMELL) [0 Code Smells](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-core&pullRequest=8&resolved=false&types=CODE_SMELL)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/CoverageChart/25.png' alt='33.3%' width='16' height='16' />](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-scripting-core&pullRequest=8&metric=new_coverage&view=list) [33.3% Coverage](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-scripting-core&pullRequest=8&metric=new_coverage&view=list)
[<img src='https://sonarsource.github.io/sonarcloud-github-static-resources/v2/checks/Duplications/3.png' alt='0.0%' width='16' height='16' />](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-scripting-core&pullRequest=8&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-scripting-core&pullRequest=8&metric=new_duplicated_lines_density&view=list)
--
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: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-scripting-core] sonarcloud[bot] commented on pull request #8: SLING-10584 log an info message if adding bindings takes more than 1 msec
Posted by GitBox <gi...@apache.org>.
sonarcloud[bot] commented on pull request #8:
URL: https://github.com/apache/sling-org-apache-sling-scripting-core/pull/8#issuecomment-892462489
SonarCloud Quality Gate failed. 
[](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-core&pullRequest=8&resolved=false&types=BUG) [](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-core&pullRequest=8&resolved=false&types=BUG) [0 Bugs](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-core&pullRequest=8&resolved=false&types=BUG)
[](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-core&pullRequest=8&resolved=false&types=VULNERABILITY) [](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-core&pullRequest=8&resolved=false&types=VULNERABILITY) [0 Vulnerabilities](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-core&pullRequest=8&resolved=false&types=VULNERABILITY)
[](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-scripting-core&pullRequest=8&resolved=false&types=SECURITY_HOTSPOT) [](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-scripting-core&pullRequest=8&resolved=false&types=SECURITY_HOTSPOT) [0 Security Hotspots](https://sonarcloud.io/project/security_hotspots?id=apache_sling-org-apache-sling-scripting-core&pullRequest=8&resolved=false&types=SECURITY_HOTSPOT)
[](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-core&pullRequest=8&resolved=false&types=CODE_SMELL) [](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-core&pullRequest=8&resolved=false&types=CODE_SMELL) [2 Code Smells](https://sonarcloud.io/project/issues?id=apache_sling-org-apache-sling-scripting-core&pullRequest=8&resolved=false&types=CODE_SMELL)
[](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-scripting-core&pullRequest=8&metric=new_coverage&view=list) [52.6% Coverage](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-scripting-core&pullRequest=8&metric=new_coverage&view=list)
[](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-scripting-core&pullRequest=8&metric=new_duplicated_lines_density&view=list) [0.0% Duplication](https://sonarcloud.io/component_measures?id=apache_sling-org-apache-sling-scripting-core&pullRequest=8&metric=new_duplicated_lines_density&view=list)
--
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: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [sling-org-apache-sling-scripting-core] joerghoh commented on pull request #8: SLING-10584 log an info message if adding bindings takes more than 1 msec
Posted by GitBox <gi...@apache.org>.
joerghoh commented on pull request #8:
URL: https://github.com/apache/sling-org-apache-sling-scripting-core/pull/8#issuecomment-891257073
Should we continue with the discussion? Otherwise I would merge this PR in the next days.
--
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: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org