Re: svn commit: r1702821 - in /tomcat/tc8.0.x/trunk: java/org/apache/tomcat/util/http/HttpMessages.java webapps/docs/changelog.xml
Mark,
On 9/14/15 4:00 AM, Mark Thomas wrote:
> On 13/09/2015 23:11, Caldarale, Charles R wrote:
>>> From: ma...@apache.org [mailto:ma...@apache.org]
>>> Subject: svn commit: r1702821 - in /tomcat/tc8.0.x/trunk:
>>> java/org/apache/tomcat/util/http/HttpMessages.java
>>> webapps/docs/changelog.xml
>>
>>> --- tomcat/tc8.0.x/trunk/java/org/apache/tomcat/util/http/HttpMessages.java
>>> (original)
>>> +++ tomcat/tc8.0.x/trunk/java/org/apache/tomcat/util/http/HttpMessages.java
>>> Sun Sep 13 20:36:40 2015
>>> @@ -69,26 +69,34 @@ public class HttpMessages {
>>> // Does HTTP requires/allow international messages or
>>> // are pre-defined? The user doesn't see them most of the time
>>> switch( status ) {
>>> -case 200:
>>> -if(st_200 == null ) {
>>> -st_200 = sm.getString("sc.200");
>>> +case 200: {
>>> +String s = st_200;
>>> +if(s == null ) {
>>> +st_200 = s = sm.getString("sc.200");
>>> }
>>> -return st_200;
>>
>> I'm not convinced this removes the data race, since there's still no
>> guarantee that the storage writes that are part of the object initialization
>> inside sm.getString() will be visible to another thread before the write to
>> st_200. Shipilёv's blog has some interesting studies:
>> http://shipilev.net/blog/2014/safe-public-construction/
>
> OK. Let me have a read of that blog.
>
> At the moment we have two people who claim to be expert in this area
> with different opinions on whether or not the above is safe. Personally,
> my money is on Chuck being right.
What's the reason for caching references to the return value of
sm.getString()? Is there a significant performance advantage?
-chris
signature.asc
Description: OpenPGP digital signature
Re: svn commit: r1702821 - in /tomcat/tc8.0.x/trunk: java/org/apache/tomcat/util/http/HttpMessages.java webapps/docs/changelog.xml
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? The way to remove race conditions from this code is to cache the values upon construction of the HttpMessages object, of course. I haven't yet read that blog post but I suspect it says that when possible, prefer doing one-time operations in constructors instead of using lazy initialization. In the case of this class, it seems that lazy initialization doesn't really buy us anything: those strings will be loaded eventually anyway. -chris signature.asc Description: OpenPGP digital signature
Re: svn commit: r1702821 - in /tomcat/tc8.0.x/trunk: java/org/apache/tomcat/util/http/HttpMessages.java webapps/docs/changelog.xml
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. 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. > > 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 ... ? Mark - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
RE: svn commit: r1702821 - in /tomcat/tc8.0.x/trunk: java/org/apache/tomcat/util/http/HttpMessages.java webapps/docs/changelog.xml
> 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. - Chuck THIS COMMUNICATION MAY CONTAIN CONFIDENTIAL AND/OR OTHERWISE PROPRIETARY MATERIAL and is thus for use only by the intended recipient. If you received this in error, please contact the sender and delete the e-mail and its attachments from all computers. - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
RE: svn commit: r1702821 - in /tomcat/tc8.0.x/trunk: java/org/apache/tomcat/util/http/HttpMessages.java webapps/docs/changelog.xml
> 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 > > 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. You're presuming that java.util.ResourceBundle has been implemented the way you'd like and doesn't use weak references or something similar. But yes, in the typical case, this shouldn't be much more than a map lookup. - Chuck THIS COMMUNICATION MAY CONTAIN CONFIDENTIAL AND/OR OTHERWISE PROPRIETARY MATERIAL and is thus for use only by the intended recipient. If you received this in error, please contact the sender and delete the e-mail and its attachments from all computers. - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: svn commit: r1702821 - in /tomcat/tc8.0.x/trunk: java/org/apache/tomcat/util/http/HttpMessages.java webapps/docs/changelog.xml
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
Re: svn commit: r1702821 - in /tomcat/tc8.0.x/trunk: java/org/apache/tomcat/util/http/HttpMessages.java webapps/docs/changelog.xml
On 15/09/2015 20:37, Christopher Schultz wrote: > On 9/15/15 3:09 PM, Mark Thomas wrote: >> 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. I prefer the cleaner code too. At the moment I think there was an issue but I'm watching the discussion between those that have a better understanding of this issue than I do with a great deal of interest. I'm certainly learning a lot just from reading along. > 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. +1 What makes some of the Tomcat code interesting is optimisations that made sense back in the days of Tomcat 4 (Java 1.3) may not apply today. > 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. Makes sense. I can't think of anything like that off-hand in Tomcat but +1 to the idea. Hmm. Some of the HTTP/2 priority stuff looks a bit like that. Something to keep in mind... Thanks. Mark - To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: svn commit: r1702821 - in /tomcat/tc8.0.x/trunk: java/org/apache/tomcat/util/http/HttpMessages.java webapps/docs/changelog.xml
On 13/09/2015 23:11, Caldarale, Charles R wrote:
>> From: ma...@apache.org [mailto:ma...@apache.org]
>> Subject: svn commit: r1702821 - in /tomcat/tc8.0.x/trunk:
>> java/org/apache/tomcat/util/http/HttpMessages.java webapps/docs/changelog.xml
>
>> --- tomcat/tc8.0.x/trunk/java/org/apache/tomcat/util/http/HttpMessages.java
>> (original)
>> +++ tomcat/tc8.0.x/trunk/java/org/apache/tomcat/util/http/HttpMessages.java
>> Sun Sep 13 20:36:40 2015
>> @@ -69,26 +69,34 @@ public class HttpMessages {
>> // Does HTTP requires/allow international messages or
>> // are pre-defined? The user doesn't see them most of the time
>> switch( status ) {
>> -case 200:
>> -if(st_200 == null ) {
>> -st_200 = sm.getString("sc.200");
>> +case 200: {
>> +String s = st_200;
>> +if(s == null ) {
>> +st_200 = s = sm.getString("sc.200");
>> }
>> -return st_200;
>
> I'm not convinced this removes the data race, since there's still no
> guarantee that the storage writes that are part of the object initialization
> inside sm.getString() will be visible to another thread before the write to
> st_200. Shipilёv's blog has some interesting studies:
> http://shipilev.net/blog/2014/safe-public-construction/
OK. Let me have a read of that blog.
At the moment we have two people who claim to be expert in this area
with different opinions on whether or not the above is safe. Personally,
my money is on Chuck being right.
I don't know the JMM well enough to have a definitive opinion at this
point. I suspect that is going to have to change. This is why I *really*
want to see an explanation if why a data race occurs here - with
references to the JMM to back it up - not only to justify the work in
this area but to help those who are new to the JMM to understand exactly
why this is a problem. The more of us that understand this, the more
likely we are as a community to avoid issues like this in the future
and/or to spot them if one of us makes a mistake.
> P.S. The formatting looks a little odd here, with no space after an "if" nor
> its opening parenthesis, but a space before the closing one.
I spotted that and meant to fix it but got distracted and forgot. It
looks like I'm going to have to revist this code so I'll get that fixed
as well.
> Also, the braces on the case statements aren't needed; using them results in
> having two closing braces in the same column at the end of the switch.
The braces are required because of the repeat declarations of String s.
Mark
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
Re: svn commit: r1702821 - in /tomcat/tc8.0.x/trunk: java/org/apache/tomcat/util/http/HttpMessages.java webapps/docs/changelog.xml
On 14/09/2015 09:00, Mark Thomas wrote:
> On 13/09/2015 23:11, Caldarale, Charles R wrote:
>>> From: ma...@apache.org [mailto:ma...@apache.org]
>>> Subject: svn commit: r1702821 - in /tomcat/tc8.0.x/trunk:
>>> java/org/apache/tomcat/util/http/HttpMessages.java
>>> webapps/docs/changelog.xml
>>
>>> --- tomcat/tc8.0.x/trunk/java/org/apache/tomcat/util/http/HttpMessages.java
>>> (original)
>>> +++ tomcat/tc8.0.x/trunk/java/org/apache/tomcat/util/http/HttpMessages.java
>>> Sun Sep 13 20:36:40 2015
>>> @@ -69,26 +69,34 @@ public class HttpMessages {
>>> // Does HTTP requires/allow international messages or
>>> // are pre-defined? The user doesn't see them most of the time
>>> switch( status ) {
>>> -case 200:
>>> -if(st_200 == null ) {
>>> -st_200 = sm.getString("sc.200");
>>> +case 200: {
>>> +String s = st_200;
>>> +if(s == null ) {
>>> +st_200 = s = sm.getString("sc.200");
>>> }
>>> -return st_200;
>>
>> I'm not convinced this removes the data race, since there's still no
>> guarantee that the storage writes that are part of the object initialization
>> inside sm.getString() will be visible to another thread before the write to
>> st_200. Shipilёv's blog has some interesting studies:
>> http://shipilev.net/blog/2014/safe-public-construction/
>
> OK. Let me have a read of that blog.
Thanks Chuck. That is the best explanation of all of this that I have
seen so far.
Mark
>
> At the moment we have two people who claim to be expert in this area
> with different opinions on whether or not the above is safe. Personally,
> my money is on Chuck being right.
>
> I don't know the JMM well enough to have a definitive opinion at this
> point. I suspect that is going to have to change. This is why I *really*
> want to see an explanation if why a data race occurs here - with
> references to the JMM to back it up - not only to justify the work in
> this area but to help those who are new to the JMM to understand exactly
> why this is a problem. The more of us that understand this, the more
> likely we are as a community to avoid issues like this in the future
> and/or to spot them if one of us makes a mistake.
>
>> P.S. The formatting looks a little odd here, with no space after an "if"
>> nor its opening parenthesis, but a space before the closing one.
>
> I spotted that and meant to fix it but got distracted and forgot. It
> looks like I'm going to have to revist this code so I'll get that fixed
> as well.
>
>> Also, the braces on the case statements aren't needed; using them results
>> in having two closing braces in the same column at the end of the switch.
>
> The braces are required because of the repeat declarations of String s.
>
> Mark
>
>
> -
> To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
> For additional commands, e-mail: dev-h...@tomcat.apache.org
>
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
RE: svn commit: r1702821 - in /tomcat/tc8.0.x/trunk: java/org/apache/tomcat/util/http/HttpMessages.java webapps/docs/changelog.xml
> From: ma...@apache.org [mailto:ma...@apache.org]
> Subject: svn commit: r1702821 - in /tomcat/tc8.0.x/trunk:
> java/org/apache/tomcat/util/http/HttpMessages.java webapps/docs/changelog.xml
> --- tomcat/tc8.0.x/trunk/java/org/apache/tomcat/util/http/HttpMessages.java
> (original)
> +++ tomcat/tc8.0.x/trunk/java/org/apache/tomcat/util/http/HttpMessages.java
> Sun Sep 13 20:36:40 2015
> @@ -69,26 +69,34 @@ public class HttpMessages {
> // Does HTTP requires/allow international messages or
> // are pre-defined? The user doesn't see them most of the time
> switch( status ) {
> -case 200:
> -if(st_200 == null ) {
> -st_200 = sm.getString("sc.200");
> +case 200: {
> +String s = st_200;
> +if(s == null ) {
> +st_200 = s = sm.getString("sc.200");
> }
> -return st_200;
I'm not convinced this removes the data race, since there's still no guarantee
that the storage writes that are part of the object initialization inside
sm.getString() will be visible to another thread before the write to st_200.
Shipilёv's blog has some interesting studies:
http://shipilev.net/blog/2014/safe-public-construction/
- Chuck
P.S. The formatting looks a little odd here, with no space after an "if" nor
its opening parenthesis, but a space before the closing one. Also, the braces
on the case statements aren't needed; using them results in having two closing
braces in the same column at the end of the switch.
THIS COMMUNICATION MAY CONTAIN CONFIDENTIAL AND/OR OTHERWISE PROPRIETARY
MATERIAL and is thus for use only by the intended recipient. If you received
this in error, please contact the sender and delete the e-mail and its
attachments from all computers.
-
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
