Skip to content

Fix https://bz.apache.org/bugzilla/show_bug.cgi?id=63235 #146

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Mar 14, 2019

Conversation

markt-asf
Copy link
Contributor

Implement an alternative Charset caching strategy that does not load all
Charsets at Tomcat start. This reduces start time by ~30ms and does not,
based on the performance tests included in this commit, have a negative
impact on runtime look-ups.
It does require that the names of all supported Charsets are known to
Tomcat at compile time. The code has been tested with a range of JVMs
and a unit test is provided for testing new JVMs.

@rmaucher
Copy link
Contributor

rmaucher commented Mar 9, 2019

Ok I guess. Wasn't the plan going to include hardcoding UTF-8 ?
The previous code still looks better to me (HashMap vs concurrent HashMap) but as you seem to care about resolving this now this is ok with me.

@markt-asf
Copy link
Contributor Author

Thanks for taking a look at the code.

I was concerned about the HashMap vs ConcurrentHashMap as well but in the tests included with the patch the difference was very small. Averaged over several runs it looked like ConcurrentHashMap was slower but by less than 1ns per lookup on average. That seemed small enough not to worry about to me.

In terms of special handling of UTF-8 my plan was only to pre-load the cache for UTF-8 and ISO-8859-1. I thought about adding other charsets but given ISO-8859-1 is the spec default and nearly everyone uses UTF-8 those seemed like the best place to start. Anything else I could think of just added more complexity and reduced performance.

I'm currently leaning towards applying this after the next 9.0.x tag so there is still plenty of time for folks to review and suggest improvements.

@markt-asf markt-asf force-pushed the bz-63235 branch 2 times, most recently from 18547a4 to 2b66d8e Compare March 13, 2019 10:57
Implement an alternative Charset caching strategy that does not load all
Charsets at Tomcat start. This reduces start time by ~30ms and does not,
based on the performance tests included in this commit, have a negative
impact on runtime look-ups.
It does require that the names of all supported Charsets are known to
Tomcat at compile time. The code has been tested with a range of JVMs
and a unit test is provided for testing new JVMs.
@wilkinsona
Copy link

wilkinsona commented Mar 14, 2019

In addition to the improved startup time, this change has an even bigger positive impact on Tomcat's memory footprint. With the latest 9.0 snapshot, I have an app using embedded Tomcat that will start with -Xmx13M but not with -Xmx12M. The addition of the change proposed here allows the same app to start with -Xmx9M. That ~4MB saving in heap footprint is very welcome indeed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants