Xie Xiaodong wrote: > Do we really need to synchronize on "this" rather than separate some > individual locks for better lock granularity?
Looking at the benchmarks and the times for 10,000,000 iterations even with additional competition for locks I don't think we need the complexity. I'd want to see evidence (benchmark, user report etc) that a problem exists before making more changes, particularly ones that add complexity. If you want to look at this then I'd be happy to review a patch that added benchmark to test exactly this issue. Mark > > > > 2009/6/18 <ma...@apache.org> > >> Author: markt >> Date: Thu Jun 18 09:25:00 2009 >> New Revision: 785983 >> >> URL: http://svn.apache.org/viewvc?rev=785983&view=rev >> Log: >> Expand sync within rotatable block to fix a couple of issues: >> - fileDateFormatter is a SimpleDateFormat which is not thread safe >> - the rotationLastChecked needs to be volatile to ensure we don't execute >> the sync'd block multiple times >> Although this is a sync on 'this' in log which gets called for every >> request: >> - a similar sync occurs in getDate() for every request with minimal >> performance impact >> - microbenchmarks suggest that a sync on 'this' has similar performance to >> using ThreadLocals >> >> Based on kkolinko's patch for Tomcat 5.5.x >> >> Note there remains an issue with writing to the log if the log files >> happens to be in the process of rotating >> >> Modified: >> tomcat/trunk/java/org/apache/catalina/valves/AccessLogValve.java >> >> Modified: tomcat/trunk/java/org/apache/catalina/valves/AccessLogValve.java >> URL: >> http://svn.apache.org/viewvc/tomcat/trunk/java/org/apache/catalina/valves/AccessLogValve.java?rev=785983&r1=785982&r2=785983&view=diff >> >> ============================================================================== >> --- tomcat/trunk/java/org/apache/catalina/valves/AccessLogValve.java >> (original) >> +++ tomcat/trunk/java/org/apache/catalina/valves/AccessLogValve.java Thu >> Jun 18 09:25:00 2009 >> @@ -295,7 +295,7 @@ >> /** >> * Instant when the log daily rotation was last checked. >> */ >> - private long rotationLastChecked = 0L; >> + private volatile long rotationLastChecked = 0L; >> >> /** >> * Do we check for log file existence? Helpful if an external >> @@ -649,19 +649,21 @@ >> // Only do a logfile switch check once a second, max. >> long systime = System.currentTimeMillis(); >> if ((systime - rotationLastChecked) > 1000) { >> - >> - rotationLastChecked = systime; >> - >> - // Check for a change of date >> - String tsDate = fileDateFormatter.format(new >> Date(systime)); >> - >> - // If the date has changed, switch log files >> - if (!dateStamp.equals(tsDate)) { >> - synchronized (this) { >> + synchronized(this) { >> + if ((systime - rotationLastChecked) > 1000) { >> + rotationLastChecked = systime; >> + >> + String tsDate; >> + // Check for a change of date >> + tsDate = fileDateFormatter.format(new >> Date(systime)); >> + >> + // If the date has changed, switch log files >> if (!dateStamp.equals(tsDate)) { >> - close(); >> - dateStamp = tsDate; >> - open(); >> + if (!dateStamp.equals(tsDate)) { >> + close(); >> + dateStamp = tsDate; >> + open(); >> + } >> } >> } >> } >> >> >> >> --------------------------------------------------------------------- >> 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
