Re: [PR] Reducing memory usage while evaluating reflective EL expressions [tomcat]
jengebr commented on PR #770: URL: https://github.com/apache/tomcat/pull/770#issuecomment-2450799286 Thank you so much! :) -- 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-unsubscr...@tomcat.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: [PR] Reducing memory usage while evaluating reflective EL expressions [tomcat]
markt-asf commented on PR #770: URL: https://github.com/apache/tomcat/pull/770#issuecomment-2450776553 Those were the only changes required. Fix applied to main, 11.0.1, 10.1.x and 9.0.x. -- 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-unsubscr...@tomcat.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: [PR] Reducing memory usage while evaluating reflective EL expressions [tomcat]
markt-asf closed pull request #770: Reducing memory usage while evaluating reflective EL expressions URL: https://github.com/apache/tomcat/pull/770 -- 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-unsubscr...@tomcat.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: [PR] Reducing memory usage while evaluating reflective EL expressions [tomcat]
markt-asf commented on PR #770: URL: https://github.com/apache/tomcat/pull/770#issuecomment-2450753071 I'm planning on applying this manually to main and then back-porting. So far I've made the following changes: - refactored the inner test classes to align with the rest of the package - ignored the caching code that wasn't (fully) removed - Called Util.getMethod() before returning to address some test failures caused by visibility issues Just running the full test suite before committing to check I haven't missed anything. -- 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-unsubscr...@tomcat.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: [PR] Reducing memory usage while evaluating reflective EL expressions [tomcat]
jengebr commented on PR #770: URL: https://github.com/apache/tomcat/pull/770#issuecomment-2448361853 Changes are committed as discussed, ready for re-review. Specifically: 1. Cache is removed, fast-path is added for zero-arg cases 1. Unit tests are added for: - Zero-args method lookups - One-arg method lookups - Private method lookups -- 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-unsubscr...@tomcat.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: [PR] Reducing memory usage while evaluating reflective EL expressions [tomcat]
markt-asf commented on PR #770: URL: https://github.com/apache/tomcat/pull/770#issuecomment-2448199694 Actually, it does belong to us. It isn't part of the public API. We are free to change it how we wish. If you compare our version to the equivalent code in the EL API JAR from Jakarta EE there are various differences. -- 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-unsubscr...@tomcat.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: [PR] Reducing memory usage while evaluating reflective EL expressions [tomcat]
ChristopherSchultz commented on PR #770: URL: https://github.com/apache/tomcat/pull/770#issuecomment-2448176904 > @ChristopherSchultz what prevents changes to `javax.el.Util`? That class doesn't "belong" to us. Depending upon how it changes, we might fail the TCK. It's just better to put it in something under `org.apache.whatever`. -- 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-unsubscr...@tomcat.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: [PR] Reducing memory usage while evaluating reflective EL expressions [tomcat]
jengebr commented on PR #770:
URL: https://github.com/apache/tomcat/pull/770#issuecomment-2448111787
This complexity led me to try a fallback plan, which can be faster: if the
method has zero parameters, retrieve the Method by name. This works because
there is no confusion about parameter types. Example:
```
if (paramTypes.length == 0) {
try {
Method method = clazz.getMethod(methodName, paramTypes);
return method;
} catch (NoSuchMethodException | SecurityException e) {
// fall through to broader, slower logic
}
}
```
Test data shows this is 2x faster than caching for the fast case, and has no
appreciable impact on the others. That's an excellent outcome for my
application, not sure how you feel about the general case?
--
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-unsubscr...@tomcat.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: [PR] Reducing memory usage while evaluating reflective EL expressions [tomcat]
markt-asf commented on PR #770: URL: https://github.com/apache/tomcat/pull/770#issuecomment-2448093530 I think the changes will have to go into `Util`. Something like a map of caches, keyed by class loader with appropriate use of weak references to avoid class loader leaks. -- 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-unsubscr...@tomcat.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: [PR] Reducing memory usage while evaluating reflective EL expressions [tomcat]
jengebr commented on PR #770: URL: https://github.com/apache/tomcat/pull/770#issuecomment-2448015687 Thanks, I get your points about caching scope and cleanup. @ChristopherSchultz what prevents changes to `javax.el.Util`? -- 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-unsubscr...@tomcat.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: [PR] Reducing memory usage while evaluating reflective EL expressions [tomcat]
ChristopherSchultz commented on PR #770: URL: https://github.com/apache/tomcat/pull/770#issuecomment-2447969657 > I'd also like to see the key remove from the cache when the value expires. +1 So I think your technique @jengebr is good, but the cache just needs to be pulled-up into the application scope or similar. Definitely don't cache directly in the Util class. It probably needs to be in a class that is loaded into the application's ClassLoader to be as clean as possible. Actually, I think we *must not* change the `javax.el.Util` class, right? -- 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-unsubscr...@tomcat.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: [PR] Reducing memory usage while evaluating reflective EL expressions [tomcat]
markt-asf commented on PR #770: URL: https://github.com/apache/tomcat/pull/770#issuecomment-2447798338 Any caching approach needs to consider the case of two classes in different web applications with the same name but different structures. Essentially, caching needs to be per class loader. -- 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-unsubscr...@tomcat.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: [PR] Reducing memory usage while evaluating reflective EL expressions [tomcat]
ChristopherSchultz commented on PR #770: URL: https://github.com/apache/tomcat/pull/770#issuecomment-2429756034 I was also surprised recently to discover that the JVM does not cache results of `Class.getMethods` and similar calls. Maybe it's returning a clone each time, but either way it takes time and generates garbage. So there is a (positive) GC impact of this change as well. -- 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-unsubscr...@tomcat.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
