1594368 - Query visited status of multiple URIs in the parent process at the same time.

Status: RESOLVED FIXED

(Toolkit :: Places, task, P3)

Description

[Emilio Cobos Álvarez (:emilio)]

This is a logic follow-up to bug 1593690 that would reduce the amount of I/O considerably, potentially.

Comment 1

[Alex Thayer [:alexical] (she/her)]

How expensive is this in general? You mention reducing IO - have we ever tried just maintaining a bloom filter of visited URIs to make this cheap for the common case of unvisited URIs?

Comment 2

[Emilio Cobos Álvarez (:emilio)]

(In reply to Doug Thayer [:dthayer] from comment #1)

How expensive is this in general? You mention reducing IO - have we ever tried just maintaining a bloom filter of visited URIs to make this cheap for the common case of unvisited URIs?

The problem with the bloom filter is that makes it very easy to time whether the URL is probably-visited. Which is a privacy issue as the ones I'm trying to eliminate.

Comment 3

[Alex Thayer [:alexical] (she/her)]

Oh I see. It was a bit hard to follow the chain here because of lack of access. Trying to triage for [fxperf] though.

So, we are trying to level the time it takes to compute the visited status for visited vs unvisited links, which means our best/average case performance has to go down, but just running the queries in aggregate rather than individually makes the whole process more efficient without leaking any information through query timings?

In any case, the query is done off main thread anyway, correct? So we're just affecting overall IO throughput?

(Marking as fxperf:p3 on the assumption that we're not doing major work for this on the main thread as is)

Comment 4

[Marco Bonardo [:mak]]

This is not on the main thread from quite some time, it's only about efficiency (one query VS tens)

Comment 5

[Emilio Cobos Álvarez (:emilio)]

Yeah, it also allows the notifications back to the content process to be batched, which allows smarter restyling and such.

Comment 6

[Marco Bonardo [:mak]]

Summarizing, we are currently querying each url separately, generating one query per url.
In this bug we would like to run a single query for multiple urls, and notify back in batches, instead of one notification at a time.

Comment 7

[Greg Mierzwinski [:sparky]]

:mak, we've set the performance impact to low, and added a keyword. Let us know if you think they should be set differently.

Comment 8

[Marco Bonardo [:mak]]

The problem is caching the visited statement, as the number of entries to query will be variable.
There' a few ways to solve that:

1. bind an array. For this we need to add SQLite support (carray.h), that is feasible and it supports up to char* (no wide chars). There may be a max limit for which we'll have to chunk that complicates things a bit. We can either ask Sqlite to copy over the array, or manage lifetime on our own (Sqlite needs the array to stay alive until next bind, reset or finalize).
2. bind using json (json array => memory view), this is not super safe when the input comes from the outside, like URLs in this case, and would require to build json in cpp.
3. store URLs in a memory table, on idle resolve any pending URLs, then DELETE all resolved URLs, fetching their status though a RETURNING clause. Storing URLs may again require to bind a variable number of URLs, so it may just move the problem to the insertion statement, rather than solve it.

Comment 9

[Marco Bonardo [:mak]]

Attachment: Bug 1594368 - Query visited status of multiple URIs at the same time. r=daisuke

Instead of executing one query per URI, we execute a single query per
the whole batch of URIs to be checked. The same query is also reused
in the single URI case.
Note this still notifies back links status one by one, that will be
handled separately.

Comment 10

[Marco Bonardo [:mak]]

Attachment: Bug 1594368 - Batch notify visited link status. r=emilio!,daisuke!

Comment 11

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/6107713f0780
Query visited status of multiple URIs at the same time. r=daisuke,places-reviewers

Comment 12

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fb0536e2d581
Backed out 3 changesets (bug 1594368, bug 483318) for causing multiple failures. CLOSED TREE

Comment 13

[agoloman]

Backed out for causing multiple failures.

• Backout link

• Push with failures -GTest failures

• Failure Log
  ————————————————————————————————————

• Push with failures - build bustages

• Failure Log
  ————————————————————————————————————

• Push with failures - another gtest failure

• Failure Log
  ————————————————————————————————————

• Push with failures - valgrind bustages

• Failure Log

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/119db0349a21
Query visited status of multiple URIs at the same time. r=daisuke,places-reviewers

Comment 15

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/119db0349a21

Comment 16

[Mayank Bansal]

FWIW, this bug or bug 483318 lead to a tiny 0.1% increase in NSS section sizes opt on Windows Nightly.
This is most probably below the threshold for any alert. But reporting here just as an FYI :)

Comment 17

[Marco Bonardo [:mak]]

(In reply to Mayank Bansal from comment #16)

FWIW, this bug or bug 483318 lead to a tiny 0.1% increase in NSS section sizes opt on Windows Nightly.

If this means nss lib increased a bit, that's expected as we're bundling some small additional code (the carray SQLite extension).