Closed Bug 540765 Opened 10 years ago Closed 8 years ago

[META]Favicon Service should be async

Categories

(Toolkit :: Places, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: mak, Unassigned)

References

Details

Attachments

(8 obsolete files)

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)
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)
i meant handleError, not handleResult.
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
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.
Depends on: 542943
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.
Attached patch Part 2: the stepper (obsolete) — Splinter Review
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.
Attached patch Part3: getEffectivePageStep (obsolete) — Splinter Review
Attached patch Part4: fetchDatabaseIconStep (obsolete) — Splinter Review
Attached patch Part 2: the stepper (obsolete) — Splinter Review
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.
Attachment #426658 - Attachment is obsolete: true
Attachment #427484 - Flags: review?(sdwilsh)
Attached patch Part3: getEffectivePageStep (obsolete) — Splinter Review
attaching a couple example steps
Attachment #426659 - Attachment is obsolete: true
Attached patch Part4: fetchDatabaseIconStep (obsolete) — Splinter Review
Attachment #426660 - Attachment is obsolete: true
this ends up being (pseudo-code):

nsCOMPtr<AsyncFaviconStepper> stepper = new AsyncFaviconStepper(callback);
stepper->SetSomeKnownData();
stepper->AppendStep(new SomeStep());
stepper->AppendStep(new SomeOtherStep());
stepper->Start();
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).
Attached patch amalgamation for feedback (obsolete) — Splinter Review
This is an amalgamation of the patches, to get feedback on the full approach. Clearly review will be on separated patches, one per step.
Attachment #427484 - Attachment is obsolete: true
Attachment #427485 - Attachment is obsolete: true
Attachment #427486 - Attachment is obsolete: true
Attachment #430341 - Flags: feedback?(sdwilsh)
Attachment #427484 - Flags: review?(sdwilsh)
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).
Blocks: 499828
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.
Attachment #430341 - Flags: feedback?(sdwilsh) → feedback+
i'll post final patches along the week.
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.
(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.
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 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?
(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.
Depends on: 553489
Attachment #422631 - Attachment is obsolete: true
Attachment #430341 - Attachment is obsolete: true
splitted bug 553489 to cover the first part of this work.
Depends on: 554553
No longer blocks: 522855
No longer blocks: 499828
transforming into meta, implementation bugs should block this
Assignee: mak77 → nobody
Status: ASSIGNED → NEW
Summary: Favicon Service should be async → [META]Favicon Service should be async
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.
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.
Status: NEW → RESOLVED
Closed: 8 years ago
Depends on: 676906, asyncFavicons
Keywords: dev-doc-needed
Resolution: --- → FIXED
can you just give me the contract ID?
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)
and in js you can use PlacesUtils.favicons.
Moving dev-doc-needed to the blocking bugs where it should have been :)
Keywords: dev-doc-needed
You need to log in before you can comment on or make changes to this bug.