release iterator snapshots. free up iterator slices.

[chjj]

I think I may have fixed the memory problem I was having with iterators. It looks like ReleaseSnapshot was defined but never called anywhere after an iterator was ended.

[chjj]

cc @ralphtheninja @rvagg

[ralphtheninja]

Looks good to me but I can't say that I have this fresh in my head so just gonna do some rubber ducking.

So, Iterator::IteratorEnd is called from EndWorker::Execute, which is a result of calling iter->end() in js land right? Then there's Iterator::Release which is called from EndWorker::HandleOKCallback, which does database->ReleaseIterator(id). Why do we have two similar release calls scattered in two places? ISTM that database->ReleaseSnapshot(options->snapshot) belong together with database->ReleaseIterator(id).

[chjj]

It looks like database->IteratorRelease is doing some v8 stuff, whereas delete dbIter and ReleaseSnapshot (in IteratorEnd()) does leveldb stuff, so it makes sense for those last two to be on the thread pool where database->IteratorRelease happens in OKCallback.

[chjj]

Totally not relevant, but I feel the need to share my story after all this work to find memleaks: I had an epiphany when I was writing an in-memory BST as a replacement for memdown (I wanted something iterative, and I didn't like all the recursive calls the rb tree was making). It's a destructive bst, so if I wanted to implement a real iterator, I had to take a snapshot before traversing the tree if I wanted any sort of consistency. I looked at the code and thought I should probably null the snapshotted root node once the iterator ended to hopefully free up memory faster. Then it hit me like a ton of bricks: leveldown might not be freeing up its snapshot, and sure enough, it wasn't. :)

[chjj]

Also including my latest memleak fix here. See #272. Needs review.

[chjj]

@dominictarr, would you mind testing this PR with the memleak example I wrote? I'm not seeing a leak on the native heap, but the js heap seems to be abnormally large for me (around 40mb allocated, 8-25mb used). I'm just curious whether your v8 heap is still around ~20mb with this patch.

[chjj]

Hmm, with my old code it looks like the js heap is about 7mb smaller for me. v8 is probably defensively expanding its heap because of the startHandle getting passed into persistentHandle. It might be more performant to just copy a buffer onto a new char array and unconditionally free it on destruction since that startHandle really seems to only be an optimization for Buffer keys here.

[chjj]

Alright guys, my latest commit appears to make iterators MUCH more efficient. It simply copies the Buffer (if there is one, unlikely) and avoids passing a startHandle to the Iterator object. I think it's probably a good idea to copy the buffer anyway since the Buffer memory could be altered in JS while the iterator is still executing.

The latest commit has halved the memory usage when running my test case. The previous commits in this PR used about 65mb when running that script. The latest commit reduces that to about 30mb.

[mafintosh]

@chjj @ralphtheninja this looks LGTM from me. great job everyone.

[ralphtheninja]

@chjj Great work. Lets get this released today. I just want to try it out first :)

[ralphtheninja]

1.4.5