bitly/dablooms — pull request #19 by vmg (2012-08-04 19:54:58+00:00)
Title: Some performance tweaks

HAI FRIENDLY ENGINEERS OF BITLY I COME IN PEACE

I wanted to play around with your awesome bloom library for some internal super-secret GitHub stuff (HAH), but as it turns out, I found it a tad lacking on raw performance, despite being all ballin' and stuff when it comes to memory usage.

I ran some instrumentation on the library, made some minor (major?) changes, and wanted to share them back. Here's the rationale:

### "So I was like, let me trace this, yo"
![before](http://i.imgur.com/z3p4t.png)

This is the first instrumentation I ran with the original code (unmodified). The corpus is the `words` file that ships with Mac OS 10.7:

```
$ wc /usr/share/dict/words
235886 235886 2493109 /usr/share/dict/words
```

Clearly, the two main bottlenecks are:

- 53.7% (1274.0ms) + 9.3% (221.0ms) + 2.9% (70.0ms) spent just in core hashing (without counting the hash finalization for MD5). As it turns
- More than 5% spent in `lseek`, when the code only uses `lseek` in one place... (??)

Everything else in the trace was noise, so I decided to go HAM on just these two things. The lowest hanging fruit is certainly MD5: It is not a general purpose hash function, so its use in a bloom filter is ill-advised.

### Sex & the CityHash

My first choice for a replacement was Google's [CityHash](http://code.google.com/p/cityhash/) (ayuup, I drank the Google koolaid -- I'm a moron and deserve to be mocked). I left the original commit in the branch for reference.

This simple change halved the runtime, but traces were still showing way too much time spent in hashing. The cause? Well, the bloom filter requires a pretty wide `nfunc` size for most corpuses if you want reasonable error rates, but CityHash has only two hash modes: Either 64 bit, or 128 bit. Neither of these modes is optimal for bloom filter operation.

- **64-bit** hash output is (supposedly) optimized for small strings, which is not our target corpus at GitHub, although it should perform well with the `words` file of this synthetic benchmark. In practice, we end up performing too many calls to CityHash to fill the `nfunc` spectrum because of the small output size.

- **128-bit** (also know as brain-damage-mode) performs poorly for small strings (by poorly I mean worse than other highly-optimized general purpose hashes) and doesn't offer any other specific advantages besides the adequate output word size.

To top off this disaster, CityHash doesn't really have a native "seeded mode". The seed API performs a standard hash and then an extra iteration (??) on top of the result to mix in the seed, instead of seeding the standard hash initially.

...So I killed CityHash with fire.

### Enter Murmur

MurmurHash has always been my favorite real-world hash function, and in retrospect I should have skipped City and gone straight for it.

It offers brilliant performance for all kind of string values and is always linear with string sizes without requiring special-casing for short strings. It also takes alignment issues into account.

To top it off, Murmur doesn't return the hash value on the stack/registers but writes it directly to a provided buffer. This makes it exceedingly easy to fill the `bloom->hashes` buffer with a lot of random data and perform the modularization incrementally.

``` c
for (i = 0; i < bloom->nsalts; i++, hashes += 4) {
MurmurHash3_x64_128(key, key_len, bloom->salts[i], hashes);
hashes[0] = hashes[0] % bloom->counts_per_func;
hashes[1] = hashes[1] % bloom->counts_per_func;
hashes[2] = hashes[2] % bloom->counts_per_func;
hashes[3] = hashes[3] % bloom->counts_per_func;
}
```

(note that we have aligned the `hashes` buffer to 16-bytes to prevent corner-case overflow checks). This is simple and straightforward, and makes my nipples tingle. `n` salts, and each salt throws 128 bits. Wrap'em and we're done here!

### Enlarge your files

After dropping in an optimal hash function, the instrumentation showed a hilariously high percent of time spent in the kernel performing `lseek`s. I wondered where it was coming from...

``` c
for (; size < new_size; size++) {
if (lseek(fd, size, SEEK_SET) < 0) {
perror("Error, calling lseek() to set file size");
free_bitmap(bitmap);
close(fd);
return NULL;
}
}
````

Apparently the code to resize a file on the filesystem was performing an **absolute seek** for every single byte that the file had to be increased. This is... heuh... I don't know if this is for compatibility reasons, but the POSIX standard defines a very very handy `ftruncate` call:

> The truncate() and ftruncate() functions cause the regular file named by path or referenced by fd to be truncated to a size of precisely length bytes.
>
> If the file previously was larger than this size, the extra data is lost. If the file previously was shorter, it is extended, and the extended part reads as null bytes ('\0').

This works wonders on both Mac OS X and Linux, and lets the kernel fill the file efficiently with those pesky NULL bytes, even in highly fragmented filesystems. After replacing the `lseek` calls with a `ftruncate`, all kernel operations (including the `mmap`s) became noise in the instrumentation. Awesome!

### This is where we're at now
![after](http://i.imgur.com/0Pw7o.png)

As far as I'm concerned, the instrumentation trace has been obliterated.

- Murmur cannot be made any faster, that's the way it is.
- `hash_func` is stalling with all the modulo operations (even though they have no interdeps and should be going simultaneously on the pipeline, I think...). There are no SIMD modulo instructions, so I don't see how to work around this.
- All the small bumps there come from the actual test program, not the library itself. Mostly `strchr` for splitting up the words in the dictionary file.
- `bitmap_check` and `bitmap_increment` are tiny and fast. Nothing to do here. :/
- Everything else is noise. :sparkles:

### Also, binary strings

This is not performance related (at least not directly), but it totally bummed me that the API was requiring NULL-terminated strings, specially since I'm pretty sure you wrote this to be wrapped from dynamic languages, and all these languages incur on a penalty when asking for a NULL-terminated string (see: Python string slices yo, that's some memory being duped all over the place for NULL-termination) instead of grabbing the raw buffer + it's length.

I've changed the API accordingly, adding a `len` argument to all calls. Obviously, NULL-terminated strings can still be used by passing `strlen(string)` in the external API, instead of performing the measurement internally like before.

### Final benchmarks

Averaged 8 runs for the original code, `words` is still the corpus.

```
Run 1: 2.182463s
Run 2: 2.177441s
Run 3: 2.174175s
Run 4: 2.178066s
Run 5: 2.190548s
Run 6: 2.179080s
Run 7: 2.180691s
Run 8: 2.184210s
AVG: 2.180834
```

Averaged 8 runs for the updated code, same corpus.

```
Run 1: 0.321654s
Run 2: 0.310658s
Run 3: 0.314666s
Run 4: 0.307526s
Run 5: 0.311680s
Run 6: 0.316963s
Run 7: 0.307528s
Run 8: 0.309479s
AVG: 0.312519
```

700% increase on this synthetic benchmark. For our specific corpus (bigger count, strings significantly larger than dictionary words), I get a 1300% increase. This is basically Murmur at work. Results may vary.

### Hey you piece of shit did you break the filtering?

Don't think so. Murmur generates *very* high quality entropy, high enough to come close to MD5 for all measurements.

It's on my TODO list to perform some tests and see if there's an statistically significant variance on the amount of false positives between the two hash functions. Anecdotally, for the `words` dataset, MD5 was generating 1859 positives, while Murmur decreased that to 1815. **THIS IS NOT SCIENTIFIC**.

Common sense tells us that MD5, being cryptographically sound, should always stay ahead on pure entropy measurement, but the avalanching properties of Murmur are gorgeous. So I'm happy with this. 100% Pinkie Pie approved.

### THAT'S IT

Ayup, as far as I'm concerned this now has acceptable performance to start building big stuff with it. I may look into making this even faster when I can play with more of our real-world data.

I understand these are very serious changes coming out of fucking nowhere, so I don't expect this to be merged straight away. Feel free to look at them with a magnifying glass, test it, see how it performs with your corpus (I assume they are links?), call me a moron and set my ass on :fire:... Anyway, you know how the song goes.

> Hey, I just met you, and this is crazy, but I rewrote your bloom hashes, so merge me, maybe?