Filip Hanik - Dev Lists wrote:
> I'm not sure what we are trying to do in this test, in my mind we are
> proving nothing with this test :)
The only purpose of the test is to provide a representative micro
benchmark of the current code vs your suggestion to use ThreadLocals
instead (in the 5.5.x patch file). I wanted to see how much faster
ThreadLocals would be and was surprised to see that they were slower. I
haven't given any thought to why this might be although I am curious.
> The thread safety in AccessLogValve is the fact that the formatters are
> not thread safe and can yield some funky dates showing up.
Agreed.
> And in the ideal solution its just not to wrap everything up in a
> synchronized statement.
Not agreed. This needs to be considered in context.
- Syncs aren't that expensive.
- The microbenchmarks suggest ThreadLocals are actually slightly slower
(at least on the machines I have been testing on - others may have
different results in which case I'd be interested to see what they are)
- If there is a slight delay, it won't impact the request processing
time, it will just delay the thread being released back to the pool. Any
delay harms scalability rather than response time.
- The microbenchmark suggests the times in question are so small as to
not be worth worrying about in the grand scheme of things.
However, now we have the benchmarch, if anyone wants to propose an
alternative scheme they can use the benchmark to a) test out their idea
and b) prove the relative performance improvements.
I accept the test isn't perfect. It is only meant to give an idea of
relative performance but if an alternative approach took 50% of the time
of the current one I'd +1 it in a heartbeat.
> The other thread safety issue in AccessLogValve is the the rotation of
> files, since it seems as one thread can close the file
Agreed. I have a simple patch for this. As the writer already uses a
sync, it will cost very little to put a sync around the use of the
writer to fix this.
> 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.
The comparison really isn't that expensive and the writing is buffered
by default.
> An example:
> 1. single back thread updates the currentDateString once a second.
Yep, that is an alternative solution. Obviously currentDateString would
need to be volatile but as long as only the background thread was doing
the updates you could ditch all the syncs.
There is already the background processor but this would need to be
separate as the frequency of that is user configurable (and can be
disabled).
> 2. Add the log entries to the queue, who writes out the buffer once a
> second.
The writer is buffered by default anyway.
> 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.
The purpose of the benchmark was to give us some concrete numbers to
compare different approaches. The aim was to get relative performance
numbers rather than absolute ones.
I was a little surprised that ThreadLocal seems to be slower. I still
wonder if I got the test wrong for that but I can't see anything.
> 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();
> }
> }
Yep. As I noted above, this means adding the sync to fix the issue you
identified of trying to write during roll-over is relatively low cost.
Mark
>
>
>
> 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.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@tomcat.apache.org
For additional commands, e-mail: dev-h...@tomcat.apache.org
