540765 - [META]Favicon Service should be async

Status: RESOLVED FIXED

(Toolkit :: Places, defect)

Description

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Favicon Service is sync, but based on most common operations with favicons (set a favicon, set and load a favicon, get a favicon) and the fact DB operations could be slower due to favicons being blobs, that's slow.
It is always bad for IO on the main thread, large numbers of favicons set/get can make that really bad.

This bug aims to convert most of favicons service API to be async.
Current plan is to have an optional callback to each method, called when the operation is successful, that can avoid to the implementer (And to us) the cost of registering an history observer.

Target for the change is 1.9.3 (likely FX4.0)

Comment 1

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: Part1: new callback interface and better idl docs

Looking at the API in its completeness, i this this kind of callback function could be good for all APIs (Apart ExpireAllFavicons and failedIconCache methods that can stay sync).

The plan is to add an optional input callback param to all APIs, that will be called on success. This will avoid implementers to have to register an expensive history observer.
The drawback about this is that we will just hide error conditions, since everything is async, the alternative would be to have a full callback object with an handleResult, but seriously it does not look like favicons errors have major impact on functionality, i see them more like a fire-and-forget.

All the APIs follow a common convention involving more between these steps:
* check current data for the passed in values
* fetch new icon (from network or db)
* assign icon to page
* notify observers
* callback
Not all of these points for all the APIs clearly, but they are always in this order, so could make sense to have internal helpers executing each step (where they can't be unified)

Comment 2

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

i meant handleError, not handleResult.

Comment 3

[Shawn Wilsher :sdwilsh]

Comment on attachment 422631 [details] [diff] [review]
Part1: new callback interface and better idl docs

>   /**
>-   * Retrieves the given favicon data. Throws if we don't have data.
>+   * Retrieves the given favicon data.  Throws if we don't have data.
>    *
>    * If there is no data but we have an entry for this favicon, aDataLen will
>    * be 0 and aData will be NULL.
we should maybe deprecate this?  It's sync...

looks good otherwise

Comment 4

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

well yes, (In reply to comment #3)
> >    * If there is no data but we have an entry for this favicon, aDataLen will
> >    * be 0 and aData will be NULL.
> we should maybe deprecate this?  It's sync...

well yes, once this moves to async comments will need further tweaking, for now i've just made them shorter and clearer since i wanted to check they were not saying stupid things and i was aware of possible misses in the conversion.

Comment 5

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

So, the current plan is to implement something like a stepper, make simple tasks as steps, and let the stepper walk through them. What i have so far is something like:

nsCOMPtr<AsyncFaviconStepper> stepper = new AsyncFaviconStepper(aCallback);
// Set the data we have.
stepper->SetFaviconURI(aURI);
stepper->SetPageURI(aURI);
// Create steps, they auto register with the stepper.
nsCOMPtr<ExampleStep> step1 = new GetEffectivePageForIconStep(stepper);
nsCOMPtr<ExampleStep> step2 = new CheckIconExpirationStep(stepper);
nsCOMPtr<ExampleStep> step2 = new IconFetchStep(stepper);
...
// Init the party.
stepper->Step();

I've gone through 2 or 3 different implementations, but this one seems the simpler and cleaner, i can create generic and simple enough steps, and mix them in the needed order. Each step is async and callbacks the stepper when done, it can also save data in the stepper through setters. When the stepper is done will call the nsIFaviconDataCallback with the available data. If a step fails it will notify the stepper that will just unlink all steps and bail out.

Comment 6

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: Part 2: the stepper

I'm attaching some wip patch to get feedback. i'm still iterating a bit on design (for example i think instead of new step(stepper) i'll probably go for a stepper.addStep(new step)), i had to discard some nice code separation to avoid making the code bloat but still retainin some acceptable design. the single steps are much dedicated to a single task and that makes easier to maintain them, plus being full classes they can inherit from async statements or whatever needed.

Comment 7

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: Part3: getEffectivePageStep

Comment 8

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: Part4: fetchDatabaseIconStep

Comment 9

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: Part 2: the stepper

asking for an approach-review or first-pass (depending on your time availability).

i've been able to refactor all doSetAndLoadFavicon steps into AsyncFaviconSteps, they are better separated and readable (for now i'm only copying code, removal of old sync code will come later). i still have to put them toghether to view the full picture (and ensure consistency), ideally this way i should be able to use the same approach for all the favicons service APIs. By the end of the week i should have a couple APIs converted to better evaluate what is missing.

Comment 10

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: Part3: getEffectivePageStep

attaching a couple example steps

Comment 11

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: Part4: fetchDatabaseIconStep

Comment 12

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

this ends up being (pseudo-code):

nsCOMPtr<AsyncFaviconStepper> stepper = new AsyncFaviconStepper(callback);
stepper->SetSomeKnownData();
stepper->AppendStep(new SomeStep());
stepper->AppendStep(new SomeOtherStep());
stepper->Start();

Comment 13

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Apart some ovious needed cleanup and automated tests, doSetAndLoadFaviconForPage has been successfully converted, i see icons appearing on the bookmarks toolbar on visit, and it's nice.

I will probably have to add an aStatus param to the callback, and call it regardless on failure and cancelation, to ensure testability of the various paths.  it should have default values like STATUS_OK, STATUS_FAILED, STATUS_CANCELED (even if ucanceling just means avoiding some useless work, so it could go away).  will evaluate this while writing tests (starting test-driven dev now that the basic architecture is done, i need new tests both to avoid regressing old behaviors and to avoid regressing this stuff when converting other APIs).

Comment 14

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

Attachment: amalgamation for feedback

This is an amalgamation of the patches, to get feedback on the full approach. Clearly review will be on separated patches, one per step.

Comment 15

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

part1 and the amalgamation should now apply cleanly to current trunk.

Notice:
1. this only converts doSetAndLoadFaviconForPage
2. the callback is probably going to get a new aReason short int param, for testing purposes (recognize when we did early cancel/fail fetching an icon).

Comment 16

[Shawn Wilsher :sdwilsh]

Comment on attachment 430341 [details] [diff] [review]
amalgamation for feedback

Feedback given over irc/e-mail.  This looks good I think and should be ready for review.

Comment 17

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

i'll post final patches along the week.

Comment 18

[Shawn Wilsher :sdwilsh]

I should also add that we should remove the favicon stuff from the LAZY_ADD bits in this bug too since it'll all be async.

Comment 19

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to comment #18)
> I should also add that we should remove the favicon stuff from the LAZY_ADD
> bits in this bug too since it'll all be async.

we can't. we have to add visit and LATER add favicon, since visits adding is not async, i can't kill lazy add here.

Comment 20

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

and a reason for comment 19 among other things is that we don't want to save icons for pages we don't have.

Comment 21

[Shawn Wilsher :sdwilsh]

(In reply to comment #20)
> and a reason for comment 19 among other things is that we don't want to save
> icons for pages we don't have.
In what situations could that occur?

Comment 22

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

(In reply to comment #21)
> (In reply to comment #20)
> > and a reason for comment 19 among other things is that we don't want to save
> > icons for pages we don't have.
> In what situations could that occur?

i guess extensions, pages we can't (or don't want) to save.
There is also some risk that async expiration will collect favicons.
The code to add a page to history is not so straight forward. I should replicate it in favicons service to add a mock page entry. otherwise i should create a way to add an orphan icon, put the associate request somewhere, and after the page has been added serve the associate request.
It really looks like lot of work when we can just wait a bit more and completely kill LAZY_ADD with visits.

Comment 23

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

splitted bug 553489 to cover the first part of this work.

Comment 24

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

transforming into meta, implementation bugs should block this

Comment 25

[Mike Kaply [:mkaply]]

Can someone please update:

https://developer.mozilla.org/en/XPCOM_Interface_Reference/mozIAsyncFavicons

with the implementation details (contract ID, service info) similar to:

https://developer.mozilla.org/en/nsIFaviconService

so that add-on developers can use this API?

I can't find the contract ID in the tree for this.

Comment 26

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

it's possible some bug lacked the dev-doc-needed keyword, adding it here as an additional coverage.

And marking the bug as fixed by dependencies since most APIs have an async alternative now, minor stuff can be handled apart.

Comment 27

[Mike Kaply [:mkaply]]

can you just give me the contract ID?

Comment 28

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

The old favicons service handles both the old and the new interface, the contract id didn't change, just have to QI to the interface you need (even if likely you don't even need that, since it implements ClassInfo)

Comment 29

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

and in js you can use PlacesUtils.favicons.

Comment 30

[Eric Shepherd [:sheppy]]

Moving dev-doc-needed to the blocking bugs where it should have been :)