1499738 - Tippytop: Optimize domain lookup in RemoteSettings

Status: NEW

(Firefox :: New Tab Page, enhancement, P3)

Description

[Mathieu Leplatre [:leplatrem]]

In the RS tippycollection, each record has a domain attribute, and we lookup for it by scanning the IndexedDB store.

While looking at Bug 1486980 we realized that using IndexedDB cursors iteration for filtering was not efficient. We can improve it, and will, but there is a more radical change that we can do specifically for the tippytop usecase: since there is one record per domain, we could use domains as record IDs.

The performance win is huge. Indeed, ``.get(filters: {id: "7210b4bc-81f6-4f04-976e-1acf5faeb740"}})`` takes a couple of milliseconds, whereas we've seen ``.get(filters: {domain: "amazon.es"}})`` taking more than 1 second.

The only difficulty is that Kinto (the stack behind RS) does not accept dots in records IDs. So we have to replace them with something else (eg. "-").

The plan would be:

- 1) modify your Python script to do something like this: ``record = {"id": domain.replace(".", "-"), "domain": domain}``
- 2) delete every previous entries and upload the new ones with the new IDs.
- 3) at this point, clients will re-download everything but no improvement
- 4) for this bug, submit a patch that changes the domain lookup to this: ``.get({filters: {id: domain.replace(".", "-")}});``
- 5) at this point, old clients would still use the previous lookup on the domain field, but new clients will use the ID field and have a 2ms lookup
- 6) in the Admin UI, we can set the domain field as read-only in order to avoid manual changes that could lead to inconsistencies between the ID and domain fields
- 7) at some point, if we have uptake of 100%, we can remove the domain field from the records

What do you think?

Comment 1

[Nan Jiang [:nanj]]

Thanks for the insights, :leplatrem! This is very helpful.

We can definitely adopt the plan above. Meanwhile, we can also try some other workarounds for the time being, because the tippytop feature will go live with the upcoming 63 release.

I am proposing to shrink the current tippytop collection from 1000 entries to 100. The obvious upside is that it doesn't require any code changes or uplifts. Furthermore, it will largely alleviate the 1s get call in RS. The downside is this change might affect the tippytop icon coverage to a certain extent, though the telemetry (https://sql.telemetry.mozilla.org/queries/52883#140637) suggests that it won't be a deal killer in terms of the overall user engagement. Screenshots and rich icons are still the major icon suppliers, hence it's unlikely a slight drop (we still have the top 100 around) on tippytop would affect the overall image quality in the Topsites.

Once the plan that :leplatrem proposed gets fulfilled, we can then restore the tippytop collection as the normal lineup.
 
:leplatrem, does this make sense?

:r1cky What do you think?

Comment 2

[Ricky Rosario [:rrosario, :r1cky]]

(In reply to Nan Jiang [:nanj] from comment #1)
> :r1cky What do you think?

This plan sounds good to me!

Comment 3

[Mathieu Leplatre [:leplatrem]]

> :leplatrem, does this make sense?

If it does not impact too much the user engagement, then downsizing to 100 entries makes sense. 

I'vd just submitted a patch in Bug 1486980, it's a workaround to avoid using cursors iteration. It improves greatly the situation (<70ms), but I guess it's too late to hope for uplift in 63 :|

Let me know if you need help with your script, but basically it's the same as creating/updating records except that you delete them ;)

Comment 4

[Mathieu Leplatre [:leplatrem]]

Can I help you with this?