arturobernalg opened a new pull request, #1155: URL: https://github.com/apache/tomcat/pull/1155
Fixes https://bz.apache.org/bugzilla/show_bug.cgi?id=70241 Evaluating an EL method invocation that has arguments calls Class.getMethods() every time, which copies every public method of the class. For a class with 200 public methods this allocated about 20 KB for each evaluation. The fix for BZ 69381 (#770) added a fast path for methods without arguments only, and the discussion there left the more general cache open. The concerns raised in that review were that a cache has to be per class loader and must not keep class loaders alive. This change caches the result of Class.getMethods() in a ClassValue in jakarta.el.Util and in org.apache.el.util.ReflectionUtil (the two copies of the lookup code). A ClassValue is per Class, so classes with the same name in different class loaders do not share an entry, and the entry is released with the class so the cache cannot prevent a web application class loader from being garbage collected. Nothing in the EL code calls setAccessible(), so sharing the Method objects is safe. Results with a class with 200 public methods, JDK 21, median of 5 runs of a simple benchmark that evaluates a pre-parsed expression: | Expression | Allocated before | Allocated after | Less | Time before | Time after | |---|---|---|---|---|---| | big.m150(s) | 19976 bytes | 360 bytes | 98% | 3746 ns | 1014 ns | | big.over(s), 3 overloads | 23360 bytes | 3744 bytes | 84% | 5652 ns | 2641 ns | | big.noArg() | 264 bytes | 264 bytes | 0% | no change | no change | | list.size() > 0 | 264 bytes | 264 bytes | 0% | no change | no change | The expressions without arguments use the existing fast path and are not affected. With the before and after builds run alternately the difference was 1.03 and 1.05 times, which is within the noise of the measurement. On a running Tomcat, a JSP that evaluates ${big.m150(s)} 5000 times allocated 20034 bytes for each evaluation on main and 440 bytes with this change, and the time for each evaluation after warm up went from about 5100 ns to about 1500 ns. Tests: TestMethodInvocationAllocation compares the memory allocated for each evaluation with the memory allocated by copying the methods of the class, so it does not depend on the speed of the machine. It fails on main (7056 bytes for each evaluation, 6552 bytes to copy the methods) and passes with this change, for both ValueExpression and MethodExpression. The new tests in TestUtil and TestReflectionUtil check that the class loader of an isolated class is garbage collected after the cache was used and that classes with the same name in different class loaders do not share methods. The class loader test fails if the ClassValue is replaced by a map with strong references. All 61 test classes under el (1216 tests) pass on JDK 21 and checkstyle passes. The new and changed test classes also pass on JDK 27. Notes. The cache keeps the copied Method objects of each class used this way until the class is unloaded, about 20 KB for the 200 method class. The allocation that remains for overloaded methods comes from the overload resolution, which depends on the argument values, so it is not cached. The other calls to getMethods() and getConstructors() in the EL code (lambda coercion, constructor calls, defineFunction, importStatic and bean introspection) allocate a few hundred bytes or run once, so I left them unchanged. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
