643633 - Remove form history TTL and revisit limits

Status: RESOLVED FIXED

(Firefox :: Sync, defect)

Description

[Philipp von Weitershausen [:philikon]]

Currently we limit form records to 500 [1]. First of all, that magic number in the SQL statement is a bit of a WTF by itself. But really, why don't we do a time based cut off? OMG this code is a mess.

[1] https://mxr.mozilla.org/mozilla-central/source/services/sync/modules/engines/forms.js#74

Comment 1

[Richard Newman [:rnewman]]

It's bookmarks all over again!

Comment 2

[Mike Connor [:mconnor]]

I'm not sure when we added timestamps to form history.  500 was the fix for "form history destroys perf" for 1.2.something, and it's certainly rather arbitrary.

That's just for initial upload though.  There's no cap except for TTL past that.

Comment 3

[Richard Newman [:rnewman]]

We've talked about doing this for a while. We have an arbitrary 60-day TTL, and that makes no sense.

Comment 4

[Richard Newman [:rnewman]]

Attachment: Remove TTLs from form history records.

Not strictly 'remove', but bumping this to three years seems good enough to me. If anyone has a very strong feeling that this should be a total removal, shout now!

Comment 5

[Nick Alexander :nalexander [he/him]]

Comment on attachment 8681421 [details] [diff] [review]
Remove TTLs from form history records.

Review of attachment 8681421 [details] [diff] [review]:
-----------------------------------------------------------------

I have no strong opinion on this.

Comment 6

[Mark Hammond [:markh] [:mhammond]]

Comment on attachment 8681421 [details] [diff] [review]
Remove TTLs from form history records.

Review of attachment 8681421 [details] [diff] [review]:
-----------------------------------------------------------------

SGTM.

Comment 7

[Richard Newman [:rnewman]]

https://hg.mozilla.org/integration/fx-team/rev/a095d36d85931e3d4c03627b2dae9480298ae3ea
Bug 643633 - Remove TTLs from form history records. r=markh,nalexander

Comment 8

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/a095d36d8593

Comment 9

[Richard Newman [:rnewman]]

I kinda want to get this into 44. Ryan, Mark, waddaya think?

Comment 10

[Mark Hammond [:markh] [:mhammond]]

The patch is trivial and will improve the Sync experience, so +1

Comment 11

[Richard Newman [:rnewman]]

Comment on attachment 8681421 [details] [diff] [review]
Remove TTLs from form history records.

Approval Request Comment
[Feature/regressing bug #]:
  None.

[User impact if declined]:
  Only 60 days of form history will be synced between devices.

[Describe test coverage new/current, TreeHerder]:
  None, and none needed.

[Risks and why]: 
  This is a very, very old limit, one of two that were imposed to help with performance in the Firefox 3.5/3.6 days. (The other was a 500-item limit that was removed a long time ago.)

  It's no longer relevant, but it does limit the functionality of the browser, leading to an inconsistent and worsened experience across your devices.

  We're hoping for better search history (which is based on form history) syncing in the 44/45 timeframe, so the sooner we kill this limit the better.

[String/UUID change made/needed]:
  None.

Comment 12

[Ryan Kelly [:rfkelly]]

Comment on attachment 8681421 [details] [diff] [review]
Remove TTLs from form history records.

Review of attachment 8681421 [details] [diff] [review]:
-----------------------------------------------------------------

weighing in late, but this seems fine to me

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/a095d36d8593

Comment 14

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Richard, does changing this limit have a performance impact? Does changing the limit from 60 days to 3 years mean that now the time to sync would be orders of magnitude higher? Is that expected?

Comment 15

[Carsten Book [:Tomcat]]

removing the b2g 2.5 flag since this commit has been reverted due to an incorrect merge, sorry for the confusion

Comment 16

[Nick Alexander :nalexander [he/him]]

(In reply to Ritu Kothari (:ritu) from comment #14)
> Richard, does changing this limit have a performance impact? Does changing
> the limit from 60 days to 3 years mean that now the time to sync would be
> orders of magnitude higher? Is that expected?

The impact will be that the server stores more data, and returns more data in response to certain queries.  The storage cost is ~0 -- form entries are just (key, value), and most values are *tiny*.  The data is fetched to the client essentially only on a first sync, which happens infrequently.  I'm not concerned about this.

Comment 17

[Richard Newman [:rnewman]]

(In reply to Ritu Kothari (:ritu) from comment #14)
> Richard, does changing this limit have a performance impact? Does changing
> the limit from 60 days to 3 years mean that now the time to sync would be
> orders of magnitude higher? Is that expected?

Nope. Most users will see little to no change, particularly in incremental syncs.

The servers will see a slow linear growth in stored records, probably with a small coefficient, between 60 days and 3 years -- negligible compared to history, of course. We can easily measure this by seeing how many records are TTLed right now.

New clients for accounts in which users save lots of form records -- primarily saved searches, I expect -- will see linear increases in first sync times for form history, but form history records are small, and again this'll be a drop in the bucket compared to browsing history.

Comment 18

[Ryan Kelly [:rfkelly]]

Yeah...:bobm really oughta be on this bug...

Comment 19

[Richard Newman [:rnewman]]

Note that nothing will even begin to change until 60 days after Firefox 45 is released: May 2016.

Comment 20

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8681421 [details] [diff] [review]
Remove TTLs from form history records.

Thanks Nick and Richard for the clarifications. Looks like this is not going to have a negative performance impact, let's uplift to Aurora44.

Comment 21

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Tracked for FF44.

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/1316e92edb7f

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-b2g44_v2_5/rev/1316e92edb7f