Mark, On 9/15/15 3:09 PM, Mark Thomas wrote: > On 15/09/2015 19:51, Christopher Schultz wrote: >> Chuck, >> >> On 9/15/15 2:21 PM, Caldarale, Charles R wrote: >>>> From: Christopher Schultz [mailto:ch...@christopherschultz.net] >>>> Subject: Re: svn commit: r1702821 - in /tomcat/tc8.0.x/trunk: >>>> java/org/apache/tomcat/util/http/HttpMessages.java >>>> webapps/docs/changelog.xml >>> >>>> What's the reason for caching references to the return value of >>>> sm.getString()? Is there a significant performance advantage? >>> >>> There's a whole lot of stuff going on inside that method, including >>> string formatting based on locale. Definitely not something you want >>> to keep redoing for frequently used statuses. >> >> No string formatting, and the Locale should have already resolved by the >> time the first call was made to StringManager.getString. >> >> For a PropertyResourceBundle, this amounts to the same amount of work as >> HashMap.get(). (Though it might actually be Hashtable.get, which is a >> bit slower). >> >> So yes, it's nice to cache this value but I'm interested to see >> performance numbers. Did anyone do a benchmark already? > > Yes, I did a micro benchmark on that code. I even committed it so you > could have run the test yourself in less time that it took to wrote tis > e-mail.
:p > The cached String values are returned approximately 2 orders of > magnitude faster. OK we are talking about 10s of nano-seconds vs. > fractions of nanoseconds so it is almost certainly in the noise but I > couldn't see a good reason to drop the cache and make things > fractionally slower for every request. Agreed. Doing work where no work is required is called waste :) If the alternative was some overly-complicated mess of code, it would be worth it to take the small performance hit in favor of maintainability. In this case, the code is entirely straightforward. >> The way to remove race conditions from this code is to cache the values >> upon construction of the HttpMessages object, of course. > > And if you look at the latest version of that class you see ... ? Nothing. HttpMessages is gone from trunk. Checking 8.0 HEAD shows the problem has been resolved, but I still agree that there was only a theoretical and not an actual problem here in the first place. I like the new implementation because it's faster and, honestly, a bit more straightforward. In general, I'm not a big fan of lazy initialization unless there is a significant advantage to delaying the work or there is a very good likelihood that the work will never actually need to be done. When the work produces a hug object graph, I like to use WeakReferences so that the graph can be discarded under memory pressure and re-created multiple times during a long-running process. -chris
signature.asc
Description: OpenPGP digital signature
