(#5196) - fix excessively long replication loop

[nolanlawson]

This just contains the fix with no additional tests. Discussion in #5196 seems to indicate that a test would be very difficult to write, so I'm just opening a PR to see if it even passes Travis as-is.

[daleharvey]

So performance and replication are going to be my (and I am guessing most of pouch's) future concentrations so I am 👍 on merging this to solve the immediate issue, then hoping we have more comprehensive testing in place before it has a chance to be regressed, 👍 when green

[nolanlawson]

Yeah, I agree performance is something we should be taking more seriously. We really need tests in place for it (tests we pay attention to; I've been guilty of ignoring them for sure), otherwise we will continue regressing on perf. But I'll merge this for now!

[dharders]

Just a quick note for future reference:

• Enables the pendingBatch.changes buffer to fill up before the startNextBatch() cycle, where previously it was flushing only 1 change per batch, hence the bad performance.
• Only relevant for { live: true } 2-way replication of empty localDB with non-empty remoteDB
• This fix is much better than before, but still suffers from occasional premature batching of pendingBatch.changes.length < batch_size, even when there are batch_size + N more changes to come.

@daleharvey Concentrating on replication performance, I'd probably start with the batching immediacy issue (i.e. calls to processPendingBatch(immediate:boolean) -> 3 locations) to ensure we're only batching full buffers when it's known that more changes exist. Another important improvement to look at, although out of scope of this PR, is reducing the number of live replication xhr requests involved for a single document change. At current count, there are 11 xhr back and forths for one change, which sucks for mobile users.

@nolanlawson An idea to performance test this issue and avoid future regression:

• setup a pouchdb-express-router similar to test.read_only_replication.js
• add an app.get() route handler and count the request urls beginning with _local/
• a FAIL is numDocs < count

[dharders]

@nolanlawson I added a Performance test here

It seems to catch the behaviour quite well. Let me know what you think?

[dharders]

... At current count, there are 11 xhr back and forths for one change...

Actually, there are only 7 xhr requests for a single document update using .sync(remote, {live:true} ...

I had mistakenly disabled the cache in Chrome dev tools, which lead to extra pre-flight OPTIONS requests for CORS. Thus an extra 4 OPTIONS requests were made in my testing that would not usually occur in production. My bad!

I still think there is room for improvement to not double up check-pointing using sync().

[nolanlawson]

@dharders can you open up a new issue with that perf test, please? We should make it a permanent part of the test suite if it really susses out the error.