Monday, November 30, 2009

The demo I got most recently pulled into, involving protocols and file formats that I don't know, as well as working over a weekend, is finished. But now there's another, totally different, demo that needs to be set up by the end of the week.

On another front, the caching optimization that I've also been pulled into is being tested. First, people reviewing the code had problems with

Object lock = new Object();
synchronized (lock) {
...
}

because, based on that snippet, they said that the synchronized statement does nothing. However, if they had looked inside the synchronized statement, they would have seen the lock object being put inside a static cache, and, if they had looked at the code just outside of the snippet, they would have seen code extracting stuff from the cache. The snippet is inside the if block that occurs when extracting null from the cache. One of the else blocks would have done a synchronized on a lock object extracted from the cache to block on it instead of duplicating the work done by the thread that created the lock object.

In any case, there was a problem with the code. It seems that the code that generated the stuff in the cache also had some side-effects, so that when there was a cache hit, it would fail due to the side-effects not being done. But the caching code was being blamed, because it didn't fail in the "single-threaded case", which apparently didn't test the cache hit case.

There was a real problem with the caching code that was revealed. If the thread generating the cached data threw an exception, it would release the lock, but the lock object would still be in the cache, so other threads would endlessly loop, waiting for the first thread to replace the lock object with the cached data.

This is what the code should be with the proper clean-up:

for (;;) {
Object entry;
synchronized (cache) {
entry = cache.get(key);
}
if (entry == null) {
Object lock = new Object();
synchronized (lock) {
try {
synchronized (cache) {
if (cache.get(key) != null) {
lock = null;
continue;
}
cache.put(key, lock);
}
CacheData data = computeData(key);
synchronized (cache) {
cache.put(key, data);
}
lock = null;
return data;
} finally {
if (lock != null) {
synchronized (cache) {
cache.remove(key);
}
}
}
}
} else if (entry instanceof CacheData) {
return (CacheData) entry;
} else {
synchronized (entry) {}
}
}

Note that without the finally block, if computeData() were to throw an exception, all other lookups with that key would go into infinite loops.

The point of this code is to call computeData() only once per key, to avoid load spikes when starting up. Another approach, which was also implemented, is to preload the cache. But that code is much more uninteresting.

Unfortunately, the actual code was made more complicated, though the caching logic should be the same.

No comments:

Post a Comment