Closing trunk extremely slow

RESOLVED FIXED in Firefox 2 beta2

Status

()

RESOLVED FIXED
13 years ago
13 years ago

People

(Reporter: ria.klaassen, Assigned: regis.caspar+bz)

Tracking

({fixed1.8.1})

2.0 Branch
Firefox 2 beta2
fixed1.8.1
Points:
---
Bug Flags:
blocking-firefox2 +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 1 obsolete attachment)

- Open a trunk build
- Wait until the processor has become quiet again (about 15-20 seconds if you have some livemarks)
- Close Firefox
- Watch the contents of your profile. It takes quite some seconds until Firefox is ready for the next start
- Compare this with the yesterday's nightly

Regression between 1.9a1_2006061614 and 1.9a1_2006061622.
http://bonsai.mozilla.org/cvsquery.cgi?module=PhoenixTinderbox&date=explicit&mindate=2006-06-16+13%3A00&maxdate=2006-06-16+23%3A00

I put my money on Bug 335101 as I see nothing about places in that range.
Created attachment 225971 [details]
Screenshot task manager

Comment 2

13 years ago
I put some dump()s around the _saveSortedEngineList call in the shutdown observer for the search service and it really doesn't look like it's what's hanging us.
Ria, do you still see this?
(In reply to comment #3)
> 
Yes, it is unchanged, using this build: 1.9a1_2006062404.

Comment 5

13 years ago
Just to clarify my comment 2, although I'm not seeing that actual method hang, I am seeing it hang afterwards (with high CPU as well).

Gavin says this could be due to us having to shutdown sqlite (or something like that).
OS: Windows XP → All
Hardware: PC → All
The problem has to do with the amount of search engines.
The high flat peak on the screenshot represent 14 search engines in the profile.
When I double that number also the length of the peak is doubled.
With no engines at all in the profile I see only a quick short peak of 75% CPU usage on close.

Comment 7

13 years ago
Why is the sqlite database even being written out to on shutdown if no search engine attributes change?  With the saveSortedEngineList call commented out shutdown decreased from ~5s to ~1s.
(Assignee)

Comment 8

13 years ago
Created attachment 228275 [details] [diff] [review]
patch v1

Patch adding a engineMetadataService.settAttrs(engines, names, values) which makes a better use of transactions. (Shutdown time here from ~ 5-10s to < 1s)
(there is some commented dumps that I can remove if needed.)
Assignee: nobody → regis.caspar+bz
Status: NEW → ASSIGNED
Attachment #228275 - Flags: review?
(Assignee)

Updated

13 years ago
Attachment #228275 - Flags: review? → review?(gavin.sharp)
That patch looks good, but I was thinking it may be possible to avoid setting the order prefs at shut down if _sortedEngines wasn't touched during that session - it's only important if removeEngine or moveEngine was called, which I suspect is pretty rarely in the majority of cases. Those two functions could set a "_needToSetOrderPrefs" variable, which _saveSortedEngineList could check to see whether it needed to do anything. It may also be a good idea to set _needToSetOrderPrefs if the .filter() call in _buildSortedEngineList actually filters out nulls, to avoid it from having to do the filtering on each load, too.
Please do remove the commented out dump(), and the #bug xxx comment, and keep the starting brace in the for loop on the same line, to match the file's prevailing style.
(Assignee)

Comment 11

13 years ago
Created attachment 228278 [details] [diff] [review]
patch v2 

update regarding Gavin's comments.

(In reply to comment #9)
> That patch looks good, but I was thinking it may be possible to avoid setting
> the order prefs at shut down if _sortedEngines wasn't touched during that
> session - it's only important if removeEngine or moveEngine was called, which 
> I suspect is pretty rarely in the majority of cases. 
For sure, I was really bothered by the shutdown time beause it messes updates here (2 FF are started together...) so this patch doesn't change the logic used it's just an optimization.

Will attach an interesting screenshot showing results.
Attachment #228275 - Attachment is obsolete: true
Attachment #228278 - Flags: review?(gavin.sharp)
Attachment #228275 - Flags: review?(gavin.sharp)
(Assignee)

Comment 12

13 years ago
Created attachment 228279 [details]
interesting screenshot showing results of patch

This screenshot is a capture of process explorer monitoring CPU of firefox.exe process. On left, the official nightly i.e. Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) Gecko/20060705 Minefield/3.0a1 ID:2006070504 [cairo] with a shutdown time of around 60 secs. On right, the same build with the patch applied showing a shutdown time of ~ 5 secs.
Note that times are rather longer than usual because of process explorer (and probably winamp) taking CPU. However, both capture where made in the same conditions and shutdown was initiated after some CPU spikes (bug 337762) so that no starting operation would bias the measurement.
I just tested the latest patch, what a huge difference in shutdown speed (a few seconds compared to the normal 30++)
Attachment #228278 - Flags: review?(gavin.sharp) → review+
Is this really only a problem on the trunk? Has anybody been able to reproduce the slowness with a branch build?

Comment 15

13 years ago
On Linux I don't see any difference in shutdown times between a 2006-06-15 and a 2006-06-17 trunk build (Joey's patch landed on the 16th). Shutdown is very slow, though, but I think this is bug 341384

I don't see slow shutdowns on branch at all.

I'm really surprised that people are claiming to see such significant differences on Windows. Maybe this is Windows only?
This bug could easily be confused with Bug 338884. That bug is about closing after loading a bunch of bookmarks and https://bugzilla.mozilla.org/show_bug.cgi?id=338884#c3 is about closing *while* it is loading livemarks.
Both problems (I thought they were related some way) are still present.
(Assignee)

Comment 17

13 years ago
NB: To reproduce the slow shutdown you need some search engines (29 here).

Updated

13 years ago
Whiteboard: [checkin needed]

Comment 18

13 years ago
Regis, Gavin says this patch needs peer review before he can check it in. Asaf Romano (bugs.mano@sent) would probably be a good person to ask.
(Assignee)

Comment 19

13 years ago
Comment on attachment 228278 [details] [diff] [review]
patch v2 

(In reply to comment #18)
> Regis, Gavin says this patch needs peer review before he can check it in. Asaf
> Romano (bugs.mano@sent) would probably be a good person to ask.
OK, thanks Adam. I was wondering why this was not already checked-in.

NB: I request sr from Asaf but I'm not sure between sr and addl. review.
Attachment #228278 - Flags: superreview?(bugs.mano)
Whiteboard: [checkin needed]
Component: General → Search
QA Contact: general → search
Comment on attachment 228278 [details] [diff] [review]
patch v2 

r=mano
Attachment #228278 - Flags: superreview?(bugs.mano) → superreview+
Shall we land this on the branch as well?
Flags: blocking-firefox2?
Yes, it should land both places. I don't know if it's really critical that it gets into b1, but it wouldn't hurt.
Flags: blocking-firefox2? → blocking-firefox2+
Target Milestone: --- → Firefox 2 beta2
Whiteboard: [checkin needed]
mozilla/browser/components/search/nsSearchService.js 	1.51
Status: ASSIGNED → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED
Whiteboard: [checkin needed]
Version: Trunk → 2.0 Branch

Updated

13 years ago
Attachment #228278 - Flags: approval1.8.1? → approval1.8.1+
Comment on attachment 228278 [details] [diff] [review]
patch v2 

Approved to land a=drivers
Whiteboard: [checkin needed (1.8 branch)]
mozilla/browser/components/search/nsSearchService.js 	1.1.2.44
Keywords: fixed1.8.1
Whiteboard: [checkin needed (1.8 branch)]
You need to log in before you can comment on or make changes to this bug.