Skip to content

Conversation

@alxndrsn
Copy link
Contributor

This significantly improves performance when replicating from a PouchDB using indexeddb adapter with conflicted docs.

However, it may not be a common-enough scenario to deserve the extra complexity.

New perf test cases are included to demonstrate the improved performance. Existing test cases do not show significant change.

Example perf test runs:

                                                   | 288714d | f55b7d5
---------------------------------------------------+---------+---------
 gets-open-revs                                    |     934 |     143
 pull-replication-one-generation                   |    1165 |    1189
 pull-replication-one-generation-reverse           |     757 |     785
 pull-replication-two-generation                   |    1171 |    1139
 pull-replication-two-generation-reverse           |     752 |     769
 pull-replication-one-generation-open-revs         |   16736 |   16524
 pull-replication-one-generation-open-revs-reverse |   11872 |    5164
 pull-replication-two-generation-open-revs         |   16852 |   16831
 pull-replication-two-generation-open-revs-reverse |   11744 |    4858

alxndrsn added 2 commits October 19, 2023 12:35
This significantly improves performance when replicating a local PouchDB with
conflicted docs.

However, it may not be a common-enough scenario to deserve the extra complexity.
for (var i = 0; i < leaves.length; i++) {
var l = leaves[i];
// looks like it's the only thing couchdb checks
// TODO replace with !isValidRev(l);
Copy link
Contributor Author

@alxndrsn alxndrsn Oct 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function needs to be created.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now exists...

@alxndrsn
Copy link
Contributor Author

Tests currently failing due to body size limit in pouchdb-express-router: https://github.com/pouchdb/pouchdb-express-router/blob/2cce6eb8d9cf5f0606ef43dd66c69090aeadfa5f/lib/routes/db.js#L3

@alxndrsn
Copy link
Contributor Author

PR to increase body-parser size limits in pouchdb-express-router at pouchdb/pouchdb-express-router#16

@alxndrsn
Copy link
Contributor Author

Tests are hitting request size limits in pouchdb-express-router. PR to fix that at pouchdb/pouchdb-express-router#16

Copy link
Contributor

@garethbowen garethbowen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice improvement! The main question is about where the new code came from, which will help me understand the complexity tradeoff.


processAttachments(api, metadata, doc, opts, ctx, cb);
});
}).bind(api);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't look like code you would write... is it copied from somewhere? If so, can it be reused?

Copy link
Contributor Author

@alxndrsn alxndrsn Oct 24, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIRC it mostly came from adapter.js in pouchd-core. You're right that there's probably more code that could be shared.

var PullRequestTestObject = function () {};

PullRequestTestObject.prototype.setup = function (itr, gens) {
PullRequestTestObject.prototype.setup = function ({ itr, gens, openRevs=1, reverse=false }) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

reverse is ambiguous... initially I thought this was referring to descending. What about source="remote" or similar?

@garethbowen
Copy link
Contributor

@alxndrsn This seems fairly close to being done - is it easy enough to get it back into review?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants