576590 - Ts Shutdown (and XP Tp4?) regression from landing of async visits

Status: VERIFIED WONTFIX

(Toolkit :: Places, defect)

Description

[Marco Bonardo [:mak]]

landing bug 556400 has caused a negative change in Ts Shutdown.

My Ipothesis is that IHistory is trying to init Places (history service) earlier than usual. It was usually inited at profile-after-change, but History.cpp does not care about that nor that we have a profile setup.

The effect is that Ts Shutdown now includes in calculation init and shutdown of Places (that could even not be so bad, since we would now have better stats on improvements we make), and that xpcshell tests that don't have a profile are trying to shutdown IHistory after xpcom-shutdown (and in some case they could leak due to that)

Comment 1

[Marco Bonardo [:mak]]

at first glance the Tp4 regression looks noise... btw better waiting and see what happens of it.

Comment 2

[Marco Bonardo [:mak]]

actually, I think the point is exactly the fact we don't pass through lazy add, and this is not a real regression.
With lazy_add we add the first visit after 3 seconds, this means the first visited notification happens after 3 seconds. This means that Ts shutdown loses this notification becase shutdown comes first.
With the async patches the first visited notification comes earlier, and is not lost. This notification causes both expiration and DBFlush to start (they are in the history-observers category), and this is the additional time we see.
DBFlush will for sure flush at least once, and with a 200MB db it coul take time. Expiration could try to expire on idle, since these boxes are permanently on idle.

Comment 3

[Benjamin Stover (:stechz)]

Actually, it seems that we do not lose these changes?

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/src/nsNavHistory.cpp#5787

Comment 4

[Marco Bonardo [:mak]]

no we don't lose the changes, but now we notify causing expiration and dbflush to run (I guess Ts Shutdown hits the path through http://mxr.mozilla.org/mozilla-central/source/toolkit/components/places/src/nsNavHistory.cpp#5742 instead that shuts up notifications). I'm testing a patch that reduces the impact of expiration to practically zero when history is clean (would bea nice win regardless this bug), so that only DBflush should cause us a regression of about 30% in the worst case.
This is still not really a regression and is just due to how Ts Shutdown works.
I'm still fighting a leak in test_isVisited.js though that I have to figure out.

Remaining issues then would still be the bogus browser chrome test you addes (perma-orange) and the visited reftest (no idea how can a color: inherit test timeout randomly...)

Comment 5

[Marco Bonardo [:mak]]

I can confirm the regression is not a regression but a difference in what we measure.
LAZY_ADD was making us not measure all of Places startup and shutdown because was shutting down too early without breaking the main thread. Async addition skips LAZY_ADD, thus startup is measured completely.
The good news is that in bug 578557 I have a patche the reduces even more expiration effects, it was still acting too often in some clean case.

The remaining "regression" I measured on tryserver is about 30%, this is for the most part effect of PlacesDBFlush and initialization of other Places components (about 5% probably). We can't do anything about DBFlush right now, actually to be able to kill temp tables and DBflush we first need async visits and titles, so it's like a cursing loop, I think we should accept measuring it till we kill it.

Comment 6

[Marco Bonardo [:mak]]

We are relanding, the "regression" has been reduced as far as possible, we know what is being measured (expiration init and shutdown, dbflush init and shutdown) and it was not before, and we also have plans to reduce it (that means killing temp tables and getting rid of DBflush component, both of which require this stuff).

I'm waiting for tree-management reports, but most likely this could end up being WONTFIXed, since makes no sense having a test not measuring what is supposed to measure, and considering a more correct bracket as a regression. Tricking the test would be damn easy but no gain for anyone, I really want to see how much we can really improve the "real" Ts Shutdown killing temp tables.

Comment 7

[Marco Bonardo [:mak]]

Stats are a bit mixed up with retained layers, but luckily we know where we fail and Roc knows where his patches regress.
So avg regression is 30%, it is greater (about 100%) in micro and small profiles because the time needed to init and shutdown our components is about the same, but Ts Shutdown values for small profiles is smaller, so as a relative measure its weight is greater on smaller profiles.
I'd say that from the previous 1000% regression this is far better, especially for users, so, after all, having a measure change was useful to improve expiration.
I'm wontfixing this bug for all of the above reasons, we will improve these new numbers with further work by removing lazy_add code and temp tables.

Comment 8

[Shawn Wilsher :sdwilsh]

I'm fine with this outcome.