Is this small piece of code thread-safe?

Hi, this TimingMonitoris a class that can count the execution time of codes:

i was trying to use it in this way:

and this is the source of the class

public class TimingMonitor {

public enum Level {
		INFO, DEBUG, TRACE;
	}

private static Map<String, Long> map = Collections
			.synchronizedMap(new HashMap<String, Long>());

private final static ICILLogger logger = ICILLoggerFactory
			.getInstance(TimingMonitor.class);

private static Level logLevel = Level.DEBUG;

/**
	 * set the level of the timing log
	 * 
	 * @param level
	 *            log level
	 */
	public synchronized static void setLogLevel(Level level) {
		logLevel = level;
	}

/**
	 * mark starting point
	 * 
	 * @param marker
	 *            - name of the test
	 */
	public static void start(String marker) {
		map.put(marker, System.currentTimeMillis());
	}

/**
	 * mark ending point, printing the timing
	 * 
	 * @param marker
	 *            - name of the test
	 */
	public static void stop(String marker) {

String log = ("finish executing *" + marker + "*, total time: " + (System
				.currentTimeMillis() - map.get(marker)));

switch (logLevel) {
		case INFO:
			logger.info(log);
			break;
		case DEBUG:
			logger.debug(log);
			break;
		case TRACE:
			logger.trace(log);
			break;
		}

}
}

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().

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.

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().

Thanks,

but i think i can add some code to throw exception if users trying to do above.

then this can be avoid.