Closed Bug 382711 Opened 13 years ago Closed 12 years ago

on migration or db upgrade of a profile with livemarks, we start up the livemark service' update timer

Categories

(Firefox :: Bookmarks & History, defect, P3)

defect

Tracking

()

RESOLVED FIXED
Firefox 3 beta3

People

(Reporter: moco, Assigned: dietrich)

References

Details

(Keywords: perf)

Attachments

(1 file, 3 obsolete files)

on first time migration of a fx2 profile with livemarks, are we starting up the livemark service?

I think the answer is yes (from a simple LOG() in nsLivemarkService.js) , but I need to get a stack trace to prove it.

I'm seeing us start it up even before the first browser window comes up.

If possible, we should delay instantating the livemark service on startup like we are attempting to do in normal fx3 startup.
(In reply to comment #0)
> on first time migration of a fx2 profile with livemarks, are we starting up the
> livemark service?

Yes, I don't think we can avoid it in the initial import case. We call the livemark servce in order to create the livemark folders. Is there evidence this is causing performance problems that users actually notice?
kind of related to bug 363634, which is more about http traffic, than about service initiation.

this bug (if it is one) could also apply to the microsummary service, which we also kick off during import.
well, we probably need the livemark service in nsPlacesImportExportService.cpp
on import, see HandleLinkEnd().

but note, in nsPlacesImportExportService::nsPlacesImportExportService(), we
instantiate the livemark service, and that will set up a 15 second (seems
aggressively fast) that when it fires will update all the livemarks.

also note, there are bugs about making this less aggressive.

but I still think we should avoid doing this on import / startup.  I believe
sayrer (or mano) have done this in other parts of our code.

perhaps we want to move the code the code that starts the timer out of the
livemark service constructor, and into a method of the livemark service.

then we can start the updates of livemarks (which we will make less aggressive)
from the browser later, once we've started up and shown UI, etc.

comments?
updating summary.

to answer robert's question, I don't have numbers to show this is a real perf problem.

but you know how we have your fix for bug #363636 in toolbar.xml that duplicates LivemarkService.getSiteURI to avoid instantiating the service on startup?

http://lxr.mozilla.org/seamonkey/source/browser/components/places/content/toolbar.xml#252

Was that for a perf gain on startup?  If so, this would be related (first time migration perf startup gain).

but dietrich is right, I am trying to avoid the http traffic which I want to avoid on first time migration startup, for both livemarks and microsummaries.  (which sounds like bug #363634)
Summary: on first time migration of a fx2 profile with livemarks, are we starting up the livemark service? → on first time migration of a fx2 profile with livemarks, we start up the livemark service
(In reply to comment #4)
> 
> but you know how we have your fix for bug #363636 in toolbar.xml that
> duplicates LivemarkService.getSiteURI to avoid instantiating the service on
> startup?

Yes, it seems like the right thing to avoid instantiating the service and kicking of traffic for a normal browser startup.


> 
> http://lxr.mozilla.org/seamonkey/source/browser/components/places/content/toolbar.xml#252
> 
> Was that for a perf gain on startup?  If so, this would be related (first time
> migration perf startup gain).

iirc, it was, though I admit I didn't spend as much time measuring as I should have. My question is about the value of a "first time migration perf startup gain". I don't understand why we're focusing on that... but I admit I am totally clueless there.
"iirc, it was, though I admit I didn't spend as much time measuring as I should
have."

separate from your change, we should determine if this fix is worth the cost.
 
>My question is about the value of a "first time migration perf startup
> gain". I don't understand why we're focusing on that...

We have bugs about how first time launching trunk is really slow for people with lots of bookmarks and history.  we've got bugs on that, and I think it's bad enough where we want to make it better for the <n> millions of folks who will get upgraded to fx 3.

but note, you can run into this same issue on first start up after a schema change.  (Although eventually, we have plans not to use bookmarks.html for that, so that may not be a problem.)

I think it makes more sense to focus on making the livemark service less aggressive first, so I'll start there, before coming back to this bug.
Flags: blocking-firefox3?
Keywords: perf
OS: Windows XP → All
Hardware: PC → All
This is wanted, as that first time trying out the new Firefox will be important, but not blocking.
Flags: blocking-firefox3? → blocking-firefox3+
Whiteboard: [wanted-firefox3]
Whiteboard: [wanted-firefox3]
Target Milestone: --- → Firefox 3 M10
Attached patch fix (obsolete) — Splinter Review
here's one approach: manually start up refresh at a specified time.
Assignee: nobody → dietrich
Status: NEW → ASSIGNED
Attachment #287169 - Flags: review?(sspitzer)
(In reply to comment #7)
> This is wanted, as that first time trying out the new Firefox will be
> important, but not blocking.
> 

Hm, marked as blocking. Doesn't matter, as this effectively fixes 363634 as well (which is another blocker).
Whiteboard: [has patch][needs review sspitzer]
Priority: -- → P3
Target Milestone: Firefox 3 M10 → Firefox 3 M11
Attached patch unrotted (obsolete) — Splinter Review
this is causing problems locally when running xpcshell tests, making it difficult to test bookmarks import. i think that import starts up the livemark service update timer, and which gets shut down before fully initialized, because the test is very short.
Attachment #287169 - Attachment is obsolete: true
Attachment #294138 - Flags: review?(sspitzer)
Attachment #287169 - Flags: review?(sspitzer)
Summary: on first time migration of a fx2 profile with livemarks, we start up the livemark service → on migration or db upgrade of a profile with livemarks, we start up the livemark service' update timer
Comment on attachment 294138 [details] [diff] [review]
unrotted

chatting with dietrich over irc, this has a few issues which he is addresing.
Attachment #294138 - Attachment is obsolete: true
Attachment #294138 - Flags: review?(sspitzer)
Whiteboard: [has patch][needs review sspitzer]
Attached patch v2 (obsolete) — Splinter Review
now doesn't restart timer w/ each window load.
Attachment #294165 - Flags: review?(sspitzer)
Blocks: 384370
Comment on attachment 294165 [details] [diff] [review]
v2

r=sspitzer

I bet we could do some fancy memoization to avoid needing the the _started boolean, but I think this is fine.

one request, can you elaborate in the comment in browser.js on why we are waiting 15 seconds? 

other thoughts:

maybe we should be keeping track of the timer, and then canceling it in _shutdown()?

this._livemarkTimer = new G_Alarm();

and we could use this._livemarkTimer as our boolean, instead of _started

and then in _shutdown(), we would do:

if (this._livemarkTimer) {
  this._livemarkTimer.cancel()
  this._livemarkTimer = null;
}

do you think we'll ever have a need to stop the refresh timer, like when going offline?

all of this can be spun off, if you prefer.
Attachment #294165 - Flags: review?(sspitzer) → review+
(In reply to comment #13)
> (From update of attachment 294165 [details] [diff] [review])
> r=sspitzer
> 
> I bet we could do some fancy memoization to avoid needing the the _started
> boolean, but I think this is fine.

to do that, i'd have to make it a write-able attribute in the interface, which would be confusing IMO.

> 
> one request, can you elaborate in the comment in browser.js on why we are
> waiting 15 seconds? 

done. also, i changed to 5 seconds and moved above the DM. 15 secs was far too long.

> 
> other thoughts:
> 
> maybe we should be keeping track of the timer, and then canceling it in
> _shutdown()?
> 

done

> do you think we'll ever have a need to stop the refresh timer, like when going
> offline?
> 

i tried using ioService.offline, but it did not ever properly reflect my offline status. filed bug 410101 to follow this up.

Attached patch v3Splinter Review
Attachment #294776 - Flags: review?(sspitzer)
Attachment #294165 - Attachment is obsolete: true
Comment on attachment 294776 [details] [diff] [review]
v3

r=sspitzer
Attachment #294776 - Flags: review?(sspitzer) → review+
Blocks: 407843
i should've noted this earlier: i haven't checked this in because i think i might need to fix the livemark tests to kick off the update loop.
(In reply to comment #17)
> i should've noted this earlier: i haven't checked this in because i think i
> might need to fix the livemark tests to kick off the update loop.
> 

they explicitly reload specific livemark folders, so we should be ok.
Checking in browser/base/content/browser.js;
/cvsroot/mozilla/browser/base/content/browser.js,v  <--  browser.js
new revision: 1.927; previous revision: 1.926
done
Checking in toolkit/components/places/public/nsILivemarkService.idl;
/cvsroot/mozilla/toolkit/components/places/public/nsILivemarkService.idl,v  <--  nsILivemarkService.idl
new revision: 1.9; previous revision: 1.8
done
Checking in toolkit/components/places/src/nsLivemarkService.js;
/cvsroot/mozilla/toolkit/components/places/src/nsLivemarkService.js,v  <--  nsLivemarkService.js
new revision: 1.35; previous revision: 1.34
done
need to keep an eye out for feedback about whether we're starting the update timer too late. might need to tweak it to startup a little sooner.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Depends on: 412953
I think this caused bug 412953
Duplicate of this bug: 363634
Blocks: 390505
Bug 451915 - move Firefox/Places bugs to Firefox/Bookmarks and History. Remove all bugspam from this move by filtering for the string "places-to-b-and-h".

In Thunderbird 3.0b, you do that as follows:
Tools | Message Filters
Make sure the correct account is selected. Click "New"
Conditions: Body   contains   places-to-b-and-h
Change the action to "Delete Message".
Select "Manually Run" from the dropdown at the top.
Click OK.

Select the filter in the list, make sure "Inbox" is selected at the bottom, and click "Run Now". This should delete all the bugspam. You can then delete the filter.

Gerv
Component: Places → Bookmarks & History
QA Contact: places → bookmarks
You need to log in before you can comment on or make changes to this bug.