The default bug view has changed. See this FAQ.

move favicons blobs out of places.sqlite to their own database

ASSIGNED
Assigned to

Status

()

Toolkit
Places
P1
normal
ASSIGNED
3 years ago
a day ago

People

(Reporter: mak, Assigned: mak)

Tracking

(Blocks: 8 bugs, {perf})

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [fxsearch])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(4 attachments)

(Assignee)

Description

3 years ago
Not just cause they take space slowing down any other db access, also cause that may help with future support of larger icons.
We should definitely investigate different storage options for favicons, or even let the cache handle them, like thumbnails.
Following is a Web paage which can be found by Google Search for "sqlite size limit".
> https://www.sqlite.org/limits.html
> The current implementation will only support a string or BLOB length up to 231-1 or 2147483647

Because SQLite saves a row as a BLOB, size limit of a BLOB column is smaller than it.
Is there any plan to use favicon larger than 2GB?

From perspective of API, simplest/easiest one is localStorage.
   localStorage is constructed on mozStorage in Firefox, and Table column is "scope, key, value" only, and scope(==URI of site) is  hidden by Firefox,
   and program can update key/value pair only using setItem(key,vlaue)/removeItem(key). (other API is getItem(key), clear() only).
   value is TEXT, so, if JavaScript object, JSON.stringify is needed, and escape, base64 ecoding etc. is needed if binary data(data: ,is bse64).
   If icon data: size is less than size limit of localStorage, localStorage is most easy-to-use one for "per site data".
   This is becuse localStorage is enhancement of Cookie which is "per site data".
Second one is IndexedDB. This is also a well-designed wrapper of mozStorage. Bigest difference from localStorage is "API is asynchrounus".
Third one is mozStorage, and 4-th one is "direct use of SQLite by code himself..

What is size of "future support of larger icons"?
Even if it exceeds "vlaue .size limit of localStorage", following is pretty easy.
   var icondata = base64 data od data: URI. 
   KeyRoot="favicon"; var remain=icondata lenth; var block=65534; var Strt=0; var CNT=0;
   while(0<remain){ 
        CNT++; var Key=KeyRoot+CNT;locaalStorage.setItem(Key,icondata.substr(Start,Math.min(remain,block));
       remain=remain-block; start=start+block;
   }
As far as data size is less than value size limit of SQLite DB column, "writing data to a column of SQLite DB" is one of simplest way of "safest file writing".
     localStorage.setItem8Key,JSON.strigify(Obj)) is far better than "safe file writing of JSON file".
Even if data size is larger than value size limit of SQLite DB column, "split it to multiple pieces" is not so difficult.

Where are you planning to move favicon data?
I think localStorge is best place to hold "per site data" which is irrelevant to history relevant data.
If XUL JavaScript, following can be used.
// Directly access from XUL with any URL
   var ios = Components.classes["@mozilla.org/network/io-service;1"]
             .getService(Components.interfaces.nsIIOService);
   var ssm = Components.classes["@mozilla.org/scriptsecuritymanager;1"]
             .getService(Components.interfaces.nsIScriptSecurityManager);
   var dsm = Components.classes["@mozilla.org/dom/storagemanager;1"]
             .getService(Components.interfaces.nsIDOMStorageManager);
get_localStorage = function (Site)
{
   var uri = ios.newURI(Site, "", null);
   var principal = ssm.getCodebasePrincipal(uri);
   var localStorage = dsm.getLocalStorageForPrincipal(principal, "");
   return localStorage;
}
var URI = "favicon://www.example.com"; // traslate http://www.example.com for favicon saving. set as scope value
var localStorage=get_localStorage(URI ) ;
localStorage.setItem("favicon", data of data: uri) ; localStorage.setItem("favicon-save-time", (new Date()).toString() );
(Assignee)

Comment 2

2 years ago
(In reply to WADA from comment #1)
> Because SQLite saves a row as a BLOB, size limit of a BLOB column is smaller
> than it.
> Is there any plan to use favicon larger than 2GB?

Nope.

> From perspective of API, simplest/easiest one is localStorage.

localStorage is main-thread only, we want to do any favicon work off the main thread.

> Second one is IndexedDB. This is also a well-designed wrapper of mozStorage.
> Bigest difference from localStorage is "API is asynchrounus".


Yes that is also a possibility, but far more complex than we need.
What we need is still to have all the blobs in a single place, because we have UI views that show many favicons at once, so having blobs separated (like in a file system circle buffer) would be too expensive (opening and managing file handles is expensive).

So the idea we somehow coalesced to is to move favicons to a separate sqlite database, and ATTACH DATABASE it to places.sqlite. IT will look like part of places.sqlite, but won't cause fragmentation and size issues to it.
(Assignee)

Updated

2 years ago
Summary: move favicons blobs out of places.sqlite → move favicons blobs out of places.sqlite to their own database
FYI.
moz_favicons in places.sqlite.
> CREATE TABLE moz_favicons (  id INTEGER PRIMARY KEY, url LONGVARCHAR UNIQUE,
        data BLOB, mime_type VARCHAR(32), expiration LONG, guid TEXT)
guid is shared with moz_bookmarks?
> CREATE TABLE moz_bookmarks (  id INTEGER PRIMARY KEY, type INTEGER, fk INTEGER DEFAULT NULL, parent INTEGER, position INTEGER,
>     title LONGVARCHAR, keyword_id INTEGER, folder_type TEXT, dateAdded INTEGER, lastModified INTEGER, guid TEXT)

Mapping to localStorage.is pretty simple.
         moz_favicons + id + url => scope in localStorage,
         data => set of { key="favicon#"+1 to 2001,value=part#NN of base64 icon data }, or key="favicon",value=base64 icon data if Segments=1
                       + key="Segments",value=2001 + key="Size",value=2GB+800KB, key="BlockSize",value=1MB + key="LastSize",value=800KB
                       + key="guid",value=guid for me 
         mime-type => key=mime-type,value=mime-type of this icon
If guid is shared with moz_bookmarks.
         scope = "favicon://" + "moz_bookmarks" + ".guid" + ":9090"
         key=guid value,value=moz_favicons + id + url (pointer to scope for favicon)
(In reply to Marco Bonardo [::mak] -- catching up, delayed answers -- from comment #2)
> > From perspective of API, simplest/easiest one is localStorage.
> localStorage is main-thread only, we want to do any favicon work off the main thread.

What do you call "main-thread"?
Although localStorage is defined for JavaScript in HTML of Web site, there is no limitation  of localStoraage(==webappsstore.sqlite) use in Fx. "localStorage" is one of best Wrapper of mozStorage(==SQLite DB) with pretty simple/easy-to-use *synchrounous* API.
(IndexedDB is asynchrounous like mozStorage)
It's SQLite DB representatiion of var Obj={ A: 1, B:2, C:3, ...}.
    JavaScript Object : var dat = Obj["B"] ;                                   Obj["B"] = 1000;
    localStorage         : var dat = localStorage.getItem("B");        localStorage.setItem("B",1000);
It's a simplest one, and it's suitable to "per site data".
 
I think XUL code in Firefox is also a valid "Web Application". An XUL element can be considered "an Web site which is located in Firefox".
Please note that following is merely emulation of "window.loclStorage object creation in a window context for an URI".
> // Directly access from XUL with any URL
>    var ios = Components.classes["@mozilla.org/network/io-service;1"]
>              .getService(Components.interfaces.nsIIOService);
>    var ssm = Components.classes["@mozilla.org/scriptsecuritymanager;1"]
>              .getService(Components.interfaces.nsIScriptSecurityManager);
>    var dsm = Components.classes["@mozilla.org/dom/storagemanager;1"]
>              .getService(Components.interfaces.nsIDOMStorageManager);
> get_localStorage = function (Site)
> {
>    var uri = ios.newURI(Site, "", null);
>    var principal = ssm.getCodebasePrincipal(uri);
>    var localStorage = dsm.getLocalStorageForPrincipal(principal, "");
>    return localStorage;
> }
> var URI = "favicon://www.example.com"; // traslate http://www.example.com for favicon saving. set as scope value
> var localStorage=get_localStorage(URI ) ;
(In reply to Marco Bonardo [::mak] -- catching up, delayed answers -- from comment #2)
> So the idea we somehow coalesced to is to move favicons to a separate sqlite database, and ATTACH DATABASE it to places.sqlite.
> IT will look like part of places.sqlite, but won't cause fragmentation and size issues to it.

Do you want to split many many BLOB data for big icons to different sqlite file dur to fragmentation in places.sqlire?
If so, I think it's not so effective because SQLite holds a entire table row as a BLOB data.
If collection of many big data, repeated inset/delete produces big unused spaces in sqlite fiile, but "used spce/unused space ratio" is usually doesn't depend on block size. It merely affect on "size of unused space. "places.sqlite + favicons.sqlite + ATTACH" usually doesn't reduce "total unused disk space size in sqlite.file(s)" and "used space/unused space ration"..

Is performance issue found in places.sqlite? Or merely many complaints of "size of places.sqlite file is too big for me!" is annoying?
Biggest volume of BLOB data in places.sqlite "data for favicon"?

A known issue in SQLite is VACUUM. "Split to multiple sqlite files(DBs)" may be a good idea for "VACUUM a sqlite file at once". This kind of work can be called "user side DB split by key range or data type for performance".
    Exaample. Split table for bookmarks to multiple sqlite file(DB) by key range(for examle, range of top level domain or hashed value of URL".
                      Split data for the key range to multiple sqlite fille(DB) : sqlite file(DB) for bookmark, and sqlite file(DB) for favicon(Your idea).
(In reply to Marco Bonardo [::mak] -- catching up, delayed answers -- from comment #0)
> future support of larger icons.

"Page Thumnail as *icon* for a page" like one?

Pointer to row of central moz_places. Example of moz_bookmarks
> CREATE TABLE moz_bookmarks (
> id INTEGER PRIMARY KEY, type INTEGER, fk INTEGER DEFAULT NULL, parent INTEGER, position INTEGER,
> title LONGVARCHAR, keyword_id INTEGER, folder_type TEXT, dateAdded INTEGER, lastModified INTEGER, guid TEXT)
>   
>   id = "27" , fk = "24" , title = "Forum MozillaZine.jp Firefox"
>   colum named "flk" points id of row in moz_places.

Row in moz_favicons looks pointed only by " favicon_id" of moz_places. A kkind of "Normalization" is done properly.
> CREATE TABLE moz_places (
> id INTEGER PRIMARY KEY,
> url LONGVARCHAR, title LONGVARCHAR, (snip) 
> favicon_id INTEGER, (snip)
> 
>   id = "24" , uri = "http://forums.mozillazine.jp/viewforum.php?f=2"
>   title = "MozillaZine.jp Forum • Forum - Mozilla Firefox"
>   favicon_id = "19"
> 
> CREATE TABLE moz_favicons (
> id INTEGER PRIMARY KEY,
> url LONGVARCHAR UNIQUE, data BLOB, mime_type VARCHAR(32), expiration LONG, guid TEXT)
> 
>   id = "19" , uri = "http://forums.mozillazine.jp/favicon.ico" , data = "BLOB (Size: 608)"

If so, I think following is possible and not so hard, and it won't require big change.
  Seprated favicons.sqlite file(SQLite DB) for Table moz_favicons.
  Change definition of favicon_id = "19" of moz_places :
        id of row in Table=moz_favicons of same SQLite DB
     => id of row in Table=moz_favicons of different SQLite DB
     Because ATTACH is supported by SQLite as you say, this is not so hard.
     Consistency of "favicon_id of moz_places" can be kept by simple code in Places and/or FavIcon Manager.
       column of "users" of moz_favicons : Array of id of moz_places who referrs to this favicon.
       When moz_place uses this favico, add himself to "users".
       When moz_place stops using this favico, remove himself from "users". If no user, remove this favicon.
  Full page data, small icon for Tb, mid-size Thumnail for navigation, can be held at out side of important places.sqlite.
  If so, this DB can be used for data like Back&Forward Cache.
  If written to SQLite DB, JavaScrpt Object can be destroyed. It'll reduce memory consumption.

If SQLite DB is split, VACUUM of favicons.sqlite can be executed independently from VACUUM of places.sqlite. 
If SQLite DB is split, unbundling of "site meta data" and "site data" will be achived.
     places.sqlite    : for site meta data such as visitt history
     favicons.sqlite : for site data such as favicon, Thumnail, ...
(Assignee)

Updated

2 years ago
Priority: -- → P2
(Assignee)

Updated

2 years ago
Keywords: perf
(Assignee)

Updated

a year ago
Blocks: 492172
> or even let the cache handle them

Early versions of Mozilla just relied on the cache for favicons, but this meant that your bookmarks would randomly lose and regain their favicons as they expired from the cache / were recached, so that's not a solution I would recommend.
(Assignee)

Updated

11 months ago
Blocks: 977163
(Assignee)

Updated

9 months ago
Assignee: nobody → mak77
Priority: P2 → P1
(Assignee)

Updated

9 months ago
Depends on: 1283083
(Assignee)

Updated

9 months ago
Depends on: 1283825
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

2 months ago
Blocks: 1337397
(Assignee)

Updated

2 months ago
Blocks: 1337402
(Assignee)

Updated

2 months ago
Blocks: 1337404
(Assignee)

Updated

2 months ago
Blocks: 1337409
(Assignee)

Updated

2 months ago
Depends on: 1337829
(Assignee)

Updated

2 months ago
Blocks: 1337858
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

18 days ago
Blocks: 1346554
Status: NEW → ASSIGNED
Whiteboard: [fxsearch]
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Comment 16

13 days ago
I started posting 4 changesets that are the MVP.
This is not all the work, but it's complex enough to review, that I prefer starting sooner with that.

I plan still at least 2 changesets for this bug:
1. fallback to root icons if direct icon for a page is not found
2. more tests. I marked down parts where there's lack of tests and will write tests while the review proceeds.

In addition I need some additional testing:
3. Talos regressions
4. Migration of my default profile and verification of the results

Depending on the status of bug 1343499 I may decide to add proper ico files support here, or do that in the follow-up bug 1337402. The reason to do that here is mostly a better migration.

Comment 17

5 days ago
mozreview-review
Comment on attachment 8816669 [details]
Bug 977177 - Move favicons to a separate store.

https://reviewboard.mozilla.org/r/97320/#review125516

::: toolkit/components/places/Database.cpp:850
(Diff revision 3)
> -      rv = mMainConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING(
> +  rv = mMainConn->ExecuteSimpleSQL(NS_LITERAL_CSTRING(
> -          "PRAGMA synchronous = FULL"));
> +    MOZ_STORAGE_UNIQUIFY_QUERY_STR "PRAGMA foreign_keys = ON")
> +  );
> -      NS_ENSURE_SUCCESS(rv, rv);
> +  NS_ENSURE_SUCCESS(rv, rv);
> +#ifdef DEBUG
> +  {

I think this could use a comment: you just set the pragma, so briefly explain why you're immediately checking whether it was set.

::: toolkit/components/places/FaviconHelpers.cpp:964
(Diff revision 3)
> +  nsCOMPtr<mozIStorageStatement> stmt;
> +  nsresult rv = mDB->CreateStatement(NS_LITERAL_CSTRING(
> +    "SELECT id, width, data FROM moz_icons WHERE typeof(width) = 'text'"
> +  ), getter_AddRefs(stmt));
> +  MOZ_ASSERT(NS_SUCCEEDED(rv));
> +  if (NS_FAILED(rv)) return rv;

Nit: braces + new line as usual for if's

::: toolkit/components/places/FaviconHelpers.cpp:1017
(Diff revision 3)
> +                                           AcceptedMimeTypes::IMAGES)) {
> +    return NS_ERROR_FAILURE;
> +  }
> +
> +  // If it's an SVG, there's nothing to optimize or convert.
> +  if (aMimeType.EqualsLiteral(PNG_MIME_TYPE)) {

... this should be SVG_MIME_TYPE, right?
Attachment #8816669 - Flags: review?(adw) → review+

Comment 18

5 days ago
mozreview-review
Comment on attachment 8816670 [details]
Bug 977177 - Update favicons API consumers.

https://reviewboard.mozilla.org/r/97322/#review126032

This is probably one of the biggest patches I've ever reviewed, but I looked it over a couple of times, and I don't see any obvious problems.
Attachment #8816670 - Flags: review?(adw) → review+

Comment 19

5 days ago
mozreview-review
Comment on attachment 8848212 [details]
Bug 977177 - Add size ref fragment to icon protocols.

https://reviewboard.mozilla.org/r/121148/#review126034
Attachment #8848212 - Flags: review?(adw) → review+

Comment 20

5 days ago
mozreview-review
Comment on attachment 8848213 [details]
Bug 977177 - Expire old page to icon relations to avoid serving deprecated icons.

https://reviewboard.mozilla.org/r/121150/#review126036
Attachment #8848213 - Flags: review?(adw) → review+
Nice work Marco (as always)!  I know you said there's more to do, but congratulations on these parts!
(Assignee)

Comment 22

5 days ago
Thanks, I'll shortly start working on the next parts.
Imagelib also got support for ico frames extraction, so I may add that on top before landing the migration.
You need to log in before you can comment on or make changes to this bug.