On 27/06/2009, Xie Xiaodong <xxd82...@gmail.com> wrote: > But how could ThreadLocal be shared among all AccessLogValve instances on > the same server? After all, each thread access ThreadLocal has its own. > > Another point, based on the java doc of ThreadLocal, maybe "private > ThreadLocal<AccessDateStruct> currentDateStruct" should be declared as > static?
+1, that should ensure that it will be shared. Also it should be declared final. > > 2009/6/27 Konstantin Kolinko <knst.koli...@gmail.com> > > > > Mark Thomas wrote: > > >Konstantin Kolinko wrote: > > >> 2. struct.currentDateString is calculated in two different places > > >> I propose to encapsulate that code into AccessDateStruct > > >> > > >> 3. Invocation of getDate() in invoke() (the only place where getDate() > > >> is called): > > >> I think that getDate() also can be incapsulated into AccessDateStruct. > > >> Actually, I think of the following: > > >> 1) In invoke() there is already a variable that stores current time > > >> millis: "t2" > > >> I propose to replace that getDate() with > > >> AccessDateStruct.setDate(t2) that will perform the same job. > > >> 2) Change the signature of replace(..., Date, ..) method, and pass a > > >> reference to the AccessDateStruct instead of a Date. > > >> In the future the replace(..) methods of this class can be changed to > > >> accept a StringBuffer, instead of returning a String, but that can be > > >> another story. > > >> > > >> If AccessDateStruct is passed into the replace() method, > > >> timeTakenFormatter can be added to it as well. Maybe. > > >> > > >> 4. The log() method still creates a new Date() once in a second. > > >> The Date instance can be stored in some member variable and reused. It > > >> is in a synchronized block, so it will be safe. > > >> I do not like, that it is the same once-in-a-second synchronized > > >> block, that we already avoided once by using those ThreadLocals. > > >> Can be addressed separately, though. > > > > > > Thanks for this. I'll see about updating the patch. > > > > > > > One more thought: > > It would be better if the ThreadLocal is shared among all > > AccessLogValve instances on the same server. Otherwise, the count of > > threads will be multiplied by the count of AccessLogValve instances. > > > > That implies that the AccessDateStruct does not have any configurable > > parameters or any valve instance specific data. > > E.g., fileDateFormat is configurable, thus its formatter is not a > > candidate to be put there (and it is not there now). > > > > >> I do not like, that it is the same once-in-a-second synchronized > > >> block, that we already avoided > > I was not right writing this. We avoided sync around the formatters on > > every call, and that makes a difference. > > We already have sync in the writer on every call, so this sync > > once-in-a second won't be something noticeable. Though maybe it can be > > done once in 5/20/60 secs, but who cares. > > > > Best regards, > > Konstantin Kolinko > > > > --------------------------------------------------------------------- > > To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org > > For additional commands, e-mail: dev-h...@tomcat.apache.org > > > > > > > > -- > Sincerely yours and Best Regards, > > Xie Xiaodong > --------------------------------------------------------------------- To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org For additional commands, e-mail: dev-h...@tomcat.apache.org
