remote checkpointing generates a high number of requests (regression) #4543

Closed
willholley opened this Issue Nov 4, 2015 · 8 comments

Comments

Projects
None yet
3 participants
@willholley
Member

willholley commented Nov 4, 2015

PouchDB 5.0 changed the way that remote checkpoints are saved, using a POST to _bulk_docs instead of an explicit PUT to the checkpoint document. A side effect of this is that each POST request generates a preflight (OPTIONS) request when CORS is used, therefore doubling the number of requests used for checkpointing.

When testing in Chrome, PouchDB 4.3 would generate a preflight request for the first PUT request to a checkpoint but subsequent PUT requests do not require them.

I'd suggest reconsidering the use of _bulk_docs when only one document is being updated and/or reviewing the default frequency of checkpointing.

For comparison, replicating http://52.18.162.38:15984/movies-demo (~5000 docs):
PouchDB 4.0.3 used are 343 requests, including 125 checkpoint requests (62 GET, 62 PUT 1 OPTIONS)
PouchDB 5.0 used 375 requests, including 186 checkpoint requests (62 GET, 62 POST, 62 OPTIONS)

@daleharvey

This comment has been minimized.

Show comment
Hide comment
@daleharvey

daleharvey Nov 4, 2015

Member

Yup there are lot of individual improvements that can come out of this, thanks so much for measuring. My first question would be why are PUT requests hitting the preflight cache but POST requests are not

Member

daleharvey commented Nov 4, 2015

Yup there are lot of individual improvements that can come out of this, thanks so much for measuring. My first question would be why are PUT requests hitting the preflight cache but POST requests are not

@willholley

This comment has been minimized.

Show comment
Hide comment
@willholley

willholley Nov 4, 2015

Member

GET and POST requests are suffixed with a cache-busting parameter (_nonce=foo123) to prevent caching bugs in IE and Safari. This appears to be causing the preflight cache miss for bulk requests.

Member

willholley commented Nov 4, 2015

GET and POST requests are suffixed with a cache-busting parameter (_nonce=foo123) to prevent caching bugs in IE and Safari. This appears to be causing the preflight cache miss for bulk requests.

@nolanlawson

This comment has been minimized.

Show comment
Hide comment
@nolanlawson

nolanlawson Nov 4, 2015

Member

I see two options:

  • Remove the nonce from POSTs (would break Safari, the tests would break too I believe)
  • Revert ef0944f (or at least the part that removed PUT from http/index.js)

I prefer the latter, since the only benefit was reduced code. (We can keep multipart out as well; it's not buying us much.)

Side question: why do we need the remote checkpoints at all? I think it makes a lot more sense for clients to keep track of their own checkpoints, and it avoids weird situations with read-only databases.

Member

nolanlawson commented Nov 4, 2015

I see two options:

  • Remove the nonce from POSTs (would break Safari, the tests would break too I believe)
  • Revert ef0944f (or at least the part that removed PUT from http/index.js)

I prefer the latter, since the only benefit was reduced code. (We can keep multipart out as well; it's not buying us much.)

Side question: why do we need the remote checkpoints at all? I think it makes a lot more sense for clients to keep track of their own checkpoints, and it avoids weird situations with read-only databases.

@daleharvey

This comment has been minimized.

Show comment
Hide comment
@daleharvey

daleharvey Nov 4, 2015

Member

It wasnt just reduced code, it is cleaner code with less bugs due to duplication, we could change the behaviour back to sending PUT requests, but only if we refactor to avoid duplicating the parameter handling.

I think there is a 3rd option, reintroduce browser sniffing, if its only safari that has the caching POST and IE with GETS, removing the nonce from will allow us to use preflight caches more than we do or even did with PUT's (aside from in those browser for those requests)

We do remote checkpoints because it was a very common bug that once people wiped out their databases then replication was broken, it was filed a lot of times in various configurations (we used to only store remote, then only local etc)

Member

daleharvey commented Nov 4, 2015

It wasnt just reduced code, it is cleaner code with less bugs due to duplication, we could change the behaviour back to sending PUT requests, but only if we refactor to avoid duplicating the parameter handling.

I think there is a 3rd option, reintroduce browser sniffing, if its only safari that has the caching POST and IE with GETS, removing the nonce from will allow us to use preflight caches more than we do or even did with PUT's (aside from in those browser for those requests)

We do remote checkpoints because it was a very common bug that once people wiped out their databases then replication was broken, it was filed a lot of times in various configurations (we used to only store remote, then only local etc)

@daleharvey

This comment has been minimized.

Show comment
Hide comment
@daleharvey

daleharvey Nov 4, 2015

Member

Since the file has moved I cant see the blame, I would like to see the comments around removing the browser sniffing in the first place though before readding

Member

daleharvey commented Nov 4, 2015

Since the file has moved I cant see the blame, I would like to see the comments around removing the browser sniffing in the first place though before readding

@daleharvey

This comment has been minimized.

Show comment
Hide comment
@daleharvey

daleharvey Nov 4, 2015

Member

Ok we have never actually had browser sniffing, however when I tried to add it it wasnt trivial, #1586

@willholley is reporting that disabling the cache vastly reduces the number of requests so I think it would be great to take a bit more of an effort to reduce it to only the browsers that need it

Member

daleharvey commented Nov 4, 2015

Ok we have never actually had browser sniffing, however when I tried to add it it wasnt trivial, #1586

@willholley is reporting that disabling the cache vastly reduces the number of requests so I think it would be great to take a bit more of an effort to reduce it to only the browsers that need it

@nolanlawson

This comment has been minimized.

Show comment
Hide comment
@nolanlawson

nolanlawson Nov 4, 2015

Member

@daleharvey I would be down to add browser sniffing. IIRC, IE caches the GET but not the POST; I know Safari caches the POST but can't recall if it also caches the GET. I believe our tests will catch it if it doesn't. You might need to run the map/reduce tests in Safari/IE to be sure; I mostly recall seeing it from the POST to _query. Else I'm sure there's an _all_docs or _bulk_docs test that can be written to catch it.

We do remote checkpoints because it was a very common bug that once people wiped out their databases then replication was broken

This sounds totally reasonable to me (you destroyed your DB; what did you expect?), but we can move this discussion to another issue since it's off-topic.

Member

nolanlawson commented Nov 4, 2015

@daleharvey I would be down to add browser sniffing. IIRC, IE caches the GET but not the POST; I know Safari caches the POST but can't recall if it also caches the GET. I believe our tests will catch it if it doesn't. You might need to run the map/reduce tests in Safari/IE to be sure; I mostly recall seeing it from the POST to _query. Else I'm sure there's an _all_docs or _bulk_docs test that can be written to catch it.

We do remote checkpoints because it was a very common bug that once people wiped out their databases then replication was broken

This sounds totally reasonable to me (you destroyed your DB; what did you expect?), but we can move this discussion to another issue since it's off-topic.

@daleharvey

This comment has been minimized.

Show comment
Hide comment
@daleharvey

daleharvey Nov 13, 2015

Member

Fixed in 7ce4b66

Member

daleharvey commented Nov 13, 2015

Fixed in 7ce4b66

@daleharvey daleharvey closed this Nov 13, 2015

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