Round 1 - the Sqloop

I then picked the first major slowdown, and analysed the code further. What I found was an Sqloop. Simplified, it looked like this:

foreach ($incidents as $incident) {
    $geometry_data = $db->query('SELECT ... FROM geometry WHERE incident_id = XXX');
    // Do stuff with data
}

This is evil. As the number of incidents grows, so does the number of queries. We had over 1,000 incidents on production. So that was 1,000 queries for each time anybody zoomed the map. MySQL's query cache was probably helping to mask the effects of this, but issuing queries still has a non-zero cost.

The fix is to drag the SQL outside the loop. Get the data once, without the WHERE clause, and do the WHERE in code. It costs a little more RAM, but it's generally not a problem (and you can grab the data in chunks if you really want).

Here's the patch that I produced. You can see that it actually has little effect on the existing code. Instead of doing the SQL, it calls a function which returns exactly the same data that the SQL would have returned. That method will do a query to get data for all incidents the first time it's called, then cache it in self::$geometry_data for future calls.

I like this because it has little effect on the existing code. Because I'm not familiar with what the code should do on a high level, I'm happy with simply making sure it behaves exactly as it did before. An Ushahidi developer may have a better idea for optimising this however.

Round 2 - Iterating

A little further down, some code involving the ORM was running pretty slowly, especially given it seemed to simply be a wrapper around one query:

if (count($location_ids) > 0)
{
    $locations_result = ORM::factory('location')->in('id',implode(',',$location_ids))->find_all();
}else{
    $locations_result = ORM::factory('location')->find_all();
}

$locations = array();
foreach ($locations_result as $loc)
{
    $locations[$loc->id]['lat'] = $loc->latitude;
    $locations[$loc->id]['lon'] = $loc->longitude;
}

This segment of code was taking 270ms!

My first instinct was that the first half would be the slow bit. ORM::factory('location')->.. blah blah blah that must be slow. But splitting the timing of the two segments revealed that it was actually the second half that was crawling. What could be wrong? It's a simple foreach loop - could there be millions of iterations? Nope count($locations_result) said 510.

The only answer left was that whatever was being iterated was bad at it. It turns out the location result was not a simple array, but some kind of Iterable ORM object. I don't really know much about them, other than if they're that bad at being iterated over, we'd better not use them ;).

I patched this by doing a straight query rather than using the ORM. Again, there are probably other ways, but in this case, this worked fine. The resulting iteration on the raw array was an order of magnitude faster.

Round 3 - JSON generation

I found several instances in the code of JSON generation by string building.

Not only is this code copy/pasted several times throughout the file (leaving it open to bugs when someone fixes one part and not the others), there's an inbuilt PHP function for doing this: json_encode.

While the json generation was definitely a slow point, actually refactoring all of that code to use json_encode would have taken more time than I had available, so I ignored it. Hopefully it'll get fixed upstream some time.

Round 4 - Caching

The last thing I decided to do was implement a bit of caching.

Caching is generally the last thing you should do. If you cache before you have dealt with any other bottlenecks, you'll mask their effects, but not kill them completely. This can come back and badly bite you later.

For example, caching the JSON generation earlier would have meant we may never have spotted the wasteful sqloop, and the problem would have simply been suppressed until the site was large and active - not really a time when you want to be debugging performance issues! Also, the caching would have meant the problem would have been "intermittent" - with the poor users who get the uncached pages suffering while the others notice nothing wrong.

When you cache, you're looking to eliminate as much work as you can, preferrably without ever serving stale data.

One option is sending expires headers. This is a poor choice in this case, because the same URL can send back different data (e.g. when a new incident is added). It also gives no benefit as more visitors hit the site. They all have to request the data anyway.

In this case, it's better to cache on the server, in the action itself. The reasons for this are that we can cache once for all users, and we can generate a sensible cache key - which means we can avoid ever serving stale data.

The simplest caches are key/value stores. You put a value in the store (e.g. the JSON that the action generated), and the key you use to get it is something that you can quickly work out the next time you want the data back (e.g. the GET parameters to the script).

The trick is to generate the cache keys in such a way that if the underlying data changes, the key will change. Sometimes this means you can't just use the input parameters to the script. Here's my patch. The key is made up of the GET parameters, and the IDs of all of the incidents. It's then md5'd to make sure it only has known characters in it, and used to access a file on disk.

There are few side effects of this caching implementation:

1. As incidents are added or removed, all of the entries in the cache will effectively be worthless. But that's fine, there are many more users of the map then there are incidents being added, so we'll get benefits in most cases.
2. If an incident changes (e.g. its latitude/longitude), the cache key will NOT change, which means it won't move on the map.
3. The amount of space in /tmp that will be used is uncertain and should be monitored.

I suggest a cache cleaning implementation in the commit message that will handle most of these issues. A cronjob that looks for files that haven't been changed in the last 120 minutes and removes them will clean out the old cache files as the incidents change, and the time (120 minutes) gives a rudimentary knob with which we can manage the cache size. The only thing not handled is if incidents change, however the usual usage pattern for an Ushahidi is for events to be constantly added, and the the 120 minute limit puts an upper bound on how long the information will be wrong for anyway.