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
