I think the entire Benchmark test is wrong.
I'm not sure what we are trying to do in this test, in my mind we are
proving nothing with this test :)
The thread safety in AccessLogValve is the fact that the formatters are
not thread safe and can yield some funky dates showing up.
And in the ideal solution its just not to wrap everything up in a
synchronized statement.
The other thread safety issue in AccessLogValve is the the rotation of
files, since it seems as one thread can close the file
There are more efficient AccessLogValve, instead of doing all this
comparison crap on every single request, and writing to the file on
every single request.
An example:
1. single back thread updates the currentDateString once a second.
2. Add the log entries to the queue, who writes out the buffer once a
second.
If you don't want a background thread, then still the stuff going on in
the Benchmark test is not needed, and the bench mark is far from
efficient and there are other ways of doing it much better than we have
today.
Writing to a file the way we do it is synchronized, anyway, so the goal
was only to achieve non funky dates.
PrintWriter.java
public void println(String x) {
synchronized (lock) {
print(x);
println();
}
}
Filip
sebb wrote:
On 18/06/2009, sebb <seb...@gmail.com> wrote:
On 18/06/2009, Mark Thomas <ma...@apache.org> wrote:
> Tim Funk wrote:
> > I think this needs to be volatile ?
> > In (GetDateBenchmarkTest)
> >> + private long currentMillis = 0;
> >
> > Same for (in TimeDateElementBenchmarkTest)
> >> + private Date currentDate = null;
> >
> > Of course - since the test is single threaded - it doesn't really matter.
>
>
> The test should be multi-threaded unless I got something badly wrong. I'll
> double check.
>
> Making those volatile gets them closer to the real code. I doubt it will
make a
> difference but worth changing to be sure. You never know with these things.
The field GetDateBenchmarkTest.currentDate is set in a synch. block in
doSynch(), but for the return it is fetched outside the synch. block -
so it could potentially be changed by another thread. Also if the
synch. block is not entered, the thread might not see the correct
version of the field as there is no synch. on the read.
Similarly in TimeDateElementBenchmarkTest.getDateSync() - although the
field is volatile, it is set in the synch. block but fetched for the
return outside the block.
If it is intended to allow currentDate to be updated by another thread
before the return, then the field needs to be volatile. Otherwise the
return value needs to be established in the synch. block.
Oops, forgot to mention - there are only 5 threads in the test; it
might be useful to try tests with increasing numbers of threads to see
if the relative numbers change.
> Mark
>
>
> >
> > -Tim
> >
> > ma...@apache.org wrote:
> >> Author: markt
> >> Date: Thu Jun 18 08:32:29 2009
> >> New Revision: 785952
> >>
> >> URL: http://svn.apache.org/viewvc?rev=785952&view=rev
> >> Log:
> >> Add some micro-benchmarks that enable the differences between the Sync
> >> and ThreadLocal approach to be compared
> >
> >
> > ---------------------------------------------------------------------
> > 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
>
>
---------------------------------------------------------------------
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
