The easiest way to fix that would be to make all of those methods not be static. Then a thread would have to create a TimingMonitor instance and use that to do the timing. As it is you have something which can only time one thread at a time across the whole JVM.
That still wouldn't produce thread-safe code, but if every piece of code which needed to be timed would create its own TimingMonitor instance and avoid sharing it with any other thread, thread safety wouldn't be an issue anyway.
Another approach is to use a ThreadLocal to ensure that every TimingMonitor uses a different thread. This results in easier-to-use code, as clients don't need to create the instance and save it locally - it's just there. The only downside I see is that if you ever want to time something across different threads - i.e. start a timer in one thread and stop it in another - you can't. Normally that's not a problem, but occasionally it might be.
I believe JDK 7 and 8 will enable slicker ways to do this. But of course, they're not here yet, unless you want to use a beta of JDK 7.
Joined: Nov 15, 2009
Sergey Babkin wrote:It won't crash (thanks to the synchronized map) but it won't work right either. Think about what happens with the following sequence:
Thread A calls TimingMonitor.start("load file")
Thread B calls TimingMonitor.start("load file")
Thread A TimingMonitor.stop("load file")
Thread B calls TimingMonitor.stop("load file")
Both stop() calls will show the time since thread B called start().
but i think i can add some code to throw exception if users trying to do above.