Ria Klaassen (not reading all bugmail) Reporter
	Description • 18 years ago

- 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.

	Adam Guthrie
	Comment 2 • 18 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.

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 3 • 18 years ago

Ria, do you still see this?

	Ria Klaassen (not reading all bugmail) Reporter
	Comment 4 • 18 years ago

(In reply to comment #3)
> 
Yes, it is unchanged, using this build: 1.9a1_2006062404.

	Adam Guthrie
	Comment 5 • 18 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).

	Ria Klaassen (not reading all bugmail) Reporter
	Comment 6 • 18 years ago

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.

	Stephen Walker
	Comment 7 • 18 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.

	Régis Caspar Assignee
	Comment 8 • 18 years ago

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.)

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 9 • 18 years ago

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.

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 10 • 18 years ago

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.

	Régis Caspar Assignee
	Comment 11 • 18 years ago

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.

	Régis Caspar Assignee
	Comment 12 • 18 years ago

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.

	Peter van der Woude [:Peter6]
	Comment 13 • 18 years ago

I just tested the latest patch, what a huge difference in shutdown speed (a few seconds compared to the normal 30++)

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 14 • 18 years ago

Is this really only a problem on the trunk? Has anybody been able to reproduce the slowness with a branch build?

	Adam Guthrie
	Comment 15 • 18 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?

	Ria Klaassen (not reading all bugmail) Reporter
	Comment 16 • 18 years ago

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.

	Régis Caspar Assignee
	Comment 17 • 18 years ago

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

	Adam Guthrie
	Comment 18 • 18 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.

	Régis Caspar Assignee
	Comment 19 • 18 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.

	Asaf Romano (gone)
	Comment 20 • 18 years ago

Comment on attachment 228278 [details] [diff] [review]
patch v2 

r=mano

	Asaf Romano (gone)
	Comment 21 • 18 years ago

Shall we land this on the branch as well?

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 22 • 18 years ago

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.

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 23 • 18 years ago

mozilla/browser/components/search/nsSearchService.js 	1.51

	Mike Beltzner [:beltzner, not reading bugmail]
	Comment 24 • 18 years ago

Comment on attachment 228278 [details] [diff] [review]
patch v2 

Approved to land a=drivers

	:Gavin Sharp [email: gavin@gavinsharp.com]
	Comment 25 • 18 years ago

mozilla/browser/components/search/nsSearchService.js 	1.1.2.44