717575 - Places should use the default Storage cache size, excluding autocomplete

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Marco Bonardo [:mak]]

Actually on systems with lots of ram Places may double the cache_size limit (8MB rather than 4MB), but this may not bring interesting results.
From a first measure I did in bug 703427 Tp is unaffected, I'm going to check another couple tests on bookmarks and the locationbar, and if there's no interesting change we can proceed.

Comment 1

[Marco Bonardo [:mak]]

So, on Tp the increase is about 150 reads on 32 thousands, on a Places backup it's in noise, hard to tell if there's any increase at all.
The painful part seems to be the location bar, my simple testing (searching for 5 common words, typed slowly) shows an increase of reads from 125 thousands to 150 thousands.
This may be due to our search behavior, not having a full-text case insensitive index we have to scan all pages from the most frecent one, the cache may contain a good piece of database speeding up the first part of the scan. Though, this gain is only available to users with a database file larger than 40MB, that according to telemetry in the beta channel is not many (less than 3%).
May make more sense to fallback the usual connections, and just increase a bit the cache on the single location bar connection for anyone, at that point.

So, after falling back to the default value there are 2 choices imo:
- do nothing, people with large databases (> 40MB) will be hit by this reads increase, that is async and may not be perceivable. We could listen to feedback/telemetry. The result would be a memory usage decrease of about 3MB for those users (my Storage memory for example would move from 25MB to 22MB).
- increase the cache for the location bar connection for everyone. Will reduce reads for everyone, but increase memory for people with small databases and eventually reduce memory for others. It actually depends on the value we take, 6MB would increase memory by 2MB to ones and decrease by 2MB to others, for example.

The point is that there is no reason to differentiate on db size considering what happens, we should just unify the behavior.

Your thoughts and ideas regarding this would be appreciated.

Anything we can do to have a better IO pattern in the locationbar? Probably yes, we need to investigate a way to index words that is pluggable in SQLite, since we can't use libICU due to its size. Some time ago ddahl shown me the Tokyo Dystopia project, being able to use an indexing lib like that to limit the results and then search into a limited result set would greatly help. Though we need to investigate the possibility to use it as a virtual table or a custom tokenizer in SQLite.

Comment 2

[Justin Lebar (not reading bugmail)]

My system (with lots of RAM) reports Places as taking up 12mb:

  12.90 MB (03.13%) -- places.sqlite
    12.44 MB (03.02%) -- cache-used

But it sounds like you're saying this should be limited to 8mb?  Or do I misunderstand?

Comment 3

[Justin Lebar (not reading bugmail)]

> Though we need to investigate the possibility to use it as a virtual table or a custom tokenizer in 
> SQLite.

If we can't do it with SQLite, we can certainly write it ourselves.  You know I'm not on the anti-sqlite bandwagon, but what we do now, where we suck in the whole database every time you press a key, is nuts.  :-/

Comment 4

[Marco Bonardo [:mak]]

currently it's capped to 8MB per connection when the database size is over 40MB, or to 4MB per connection when the database size is below that. After the change would be 4MB for everyone, and we can discuss the special case of the location bar.
so to calculate you cap just check number of connections (it's the one in square brackets in about:memory) and multiply.
I wrongly talked about memory size, my fault.

(In reply to Justin Lebar [:jlebar] from comment #3)
> > Though we need to investigate the possibility to use it as a virtual table or a custom tokenizer in 
> > SQLite.
> 
> If we can't do it with SQLite, we can certainly write it ourselves.  You
> know I'm not on the anti-sqlite bandwagon, but what we do now, where we suck
> in the whole database every time you press a key, is nuts.  :-/

It's to simplify the management, we don't just search on words, but also on other data, frecency being the most important (bookmarked or tagged is another, for example). So my point is to simplify hand-off of data. We can surely search apart and then just have SQLite search a virtual table where we put the reduced set.

Comment 5

[Nicholas Nethercote [inactive]]

Here's my attempt at a summary of the current situation.

- The default cache cap for all DB connections is 4MB.
- The one exception is the Places DB, where we increase the cap to 8MB 
  if the on-disk DB is more than 40MB.
- If we removed the exceptional case, the affected people might see a 
  slowdown when typing in the location bar.
  
Is that right?

Marco is advocating we remove this exceptional case and treat everyone 
the same no matter what their on-disk DB size is.

The location bar's Places connection seems to be the most important one.  
Using a 4MB cap for the *other* Places connections definitely seems like 
a good idea, if that's possible.

Marco is also advocating that we increase the location bar's cap to 6MB, 
which will increase memory usage for those with <40MB DBs while reducing
reads, and will reduce memory usage for those with >40MB DBs with
minimal perf effect.

Comment 6

[Marco Bonardo [:mak]]

yes, that's correct!

Comment 7

[Marco Bonardo [:mak]]

Attachment: patch v1.0

Comment 5 is a good description of what this does.

Comment 8

[Dietrich Ayala (:dietrich)]

Comment on attachment 596027 [details] [diff] [review]
patch v1.0

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

sounds reasonable, r=me.

Comment 9

[Marco Bonardo [:mak]]

thanks,
https://hg.mozilla.org/integration/mozilla-inbound/rev/c63b7b310d5f

Comment 10

[Marco Bonardo [:mak]]

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