profile attached. looks like maybe in session cookies?

Indeed, it looks lie 1.5s+ is spent in SessionCookie.update.

The main culprit seems to be |_extractHostsFromEntry|, which walks recursively the history entries. This makes sense, as walking recursively history entries (to update scroll position, etc.) was already the main cost a few bugs ago. So, we need to cache this. I suspect that a good place to do so would be the TabStateCache, once we have moved it to its own module.

Now that incremental GC is working again on Nightly, did a few profiles when I saw jank and sure enough it's the same place as before: http://cl.ly/image/1u013s2B1Z3d (Whoever fixes this: would be nice to name all those anon funcs while you're in there!)

My first thought was to add a session history listener to each tab we're handling in SessionStore and handle session history additions and deletions as they happen. The problem is that in an e10s world these listeners have to live in the content script and that would mean that this wouldn't work in the current sync implementation. We could revisit that idea once we completely switched to async data collection.

The other idea is to have a separate module (in a separate JSM, possibly) that keeps track of hosts for a given window. Every time the TabStateCache is invalidated and a new list of session history entries has been collected we would need to update this cache, like: HostsPerWindow.update(tab, tabData.entries); HostsPerWindow could then retrieve the tab's parent window and update the window's list of hosts. For every hosts there could be a usage count that is increased/decreased whenever .update() is called. When the usageCount goes down to zero, this host should then not show up in the list. When a tab is closed we should make sure to remove its hosts from the list of hosts per window. When a tab is pinned/unpinned we also need to revisit the list of hosts saved for the tab because that may change depending on the privacy level settings. HostsPerWindow.retrieve(window); ... could then just return a list of hosts and flags that tells whether they're contained in a pinned tab somewhere in the window.

After playing around with some variants I think it would be best to revert some parts of bug 903388 and collect hosts while iterating through a tab's session history. We can merge them later when needed to a host list per window and can avoid going through all serialized entries again.

This patch makes SessionHistory.jsm collect hosts while serializing session history entries. They're returned with the normal tabData and can be used with sync and async data collection.

This patch makes us pass a map with window states and their IDs to SessionCookies.update(). This will be needed to retrieve the collected hosts for a given window later.

This patch introduces CookieHosts.jsm that is responsible for storing collected hosts per tab and merging multiple per-tab hosts maps to a single map for one window. SessionCookies uses the per-window map to update cookies. Per-tab host maps have to be updated whenever tab data has been collected, synchronously and asynchronously. The per-window host map however only needs to be updated whenever we collect window data. This is also how it worked before bug 903388 landed.

This patch moves SessionCookies.getHostsForWindow() to the new CookieHosts.jsm module because that's where this code should live now. This doesn't change behavior, it merely moves the code.

This should be the last part. It removes and cleans up some duplicated code related to adding entries to host maps.

Sorry for the longer list of patches but I thought they might be easier to review that way. They all should do mostly only one thing and are self-contained. Each patch building on the previous one passes the test suite, if applied in the given order.

Comment on attachment 802218 [details] [diff] [review] part 5 - Remove code duplication when adding entries to host maps Review of attachment 802218 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/sessionstore/src/CookieHosts.jsm @@ +67,5 @@ > _hostsPerWindow: new Map(), > > /** > + * Will add the given URI to the given map of hosts if it hasn't been added, > + * yet. Nit: This "will" is not really necessary. @@ +92,5 @@ > + // The given URI doesn't seem to have a host. > + return; > + } > + > + if (/https?/.test(scheme) && !hosts[host] && It is not clear to me whether /https?/ is recompiled at each call to the method. If so, you may wish to precompile it. @@ +204,5 @@ > * checkPrivacy > */ > + _extractHostsFromEntry: function (entry, hosts, isPinned) { > + try { > + this.addEntryToMap(hosts, makeURI(entry.url), isPinned, false); In the original version, |checkPrivacy| was an argument. Now, it is always |false|. Why?

Comment on attachment 802211 [details] [diff] [review] part 3 - Store and merge hosts per tab and window, and use that when updating cookies Review of attachment 802211 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/sessionstore/src/CookieHosts.jsm @@ +4,5 @@ > + > +"use strict"; > + > +this.EXPORTED_SYMBOLS = ["CookieHosts"]; > + This needs some documentation to describe the module. @@ +19,5 @@ > + }, > + > + retrieveForWindow: function (windowID) { > + return CookieHostsInternal.retrieveForWindow(windowID); > + }, Nit: |retrieve| suggests that you are doing something slow and painful. Well, at least that you're doing more than fetching from a map. Perhaps |get| would be better. @@ +31,5 @@ > + * The internal API. > + */ > +let CookieHostsInternal = { > + /** > + * A weak map with tabs as keys and their list of collected hosts as values. Nit: From this documentation, I don't understand the type of keys (is that xul:tab dom elements?) nor the values (is that an array of strings?) @@ +36,5 @@ > + */ > + _hostsPerTab: new WeakMap(), > + > + /** > + * A map with window IDs as keys and their merged list of hosts per tab. Same problem with the values. @@ +41,5 @@ > + */ > + _hostsPerWindow: new Map(), > + > + /** > + * Updates the list of collected hosts for a given tab. Nit: "... replacing the existing list" @@ +44,5 @@ > + /** > + * Updates the list of collected hosts for a given tab. > + * > + * @param tab > + * The tab owning the session history we collected the hosts from. Same issue with types, here and in the rest of the code. @@ +48,5 @@ > + * The tab owning the session history we collected the hosts from. > + * @param hosts > + * The map of hosts and their pinned states. > + */ > + updateForTab: function (tab, hosts) { What happens if I modify |hosts| after having called |updateForTab|? That's certainly not good, could you document this? @@ +61,5 @@ > + * The ID of the window to retrieve hosts for. > + * @param tabs > + * An array of tabs currently contained in the given window. > + */ > + mergeTabsToWindow: function (windowID, tabs) { I believe that the code would be more robust if we called |mergeTabsToWindow| behind the scenes, by invalidating the window whenever we call |updateTab|. This would require passing the |windowID| whenever we call |updateTab|, of course. @@ +88,5 @@ > + /** > + * Retrieves the list of hosts stored for a given window. > + * > + * @param windowID > + * The ID of the window to retrieve hosts for. Please document @return ::: browser/components/sessionstore/src/SessionStore.jsm @@ +964,5 @@ > delete this._windows[aWindow.__SSi]; > > + // Clear any collected cookie hosts. > + CookieHosts.removeForWindow(aWindow.__SSi); > + Nit: Could you update the documentation for this method? @@ +4342,5 @@ > > + // If we're still the latest async collection for the given tab and > + // the caches haven't been filled by collect() in the meantime, let's > + // fill the caches with the data we received. > + let shouldCache = this._pendingCollections.get(tab) == promise; Note to self: need to double-check this.

Comment on attachment 802169 [details] [diff] [review] part 1 - Collect hosts per tab when collecting session history Review of attachment 802169 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/sessionstore/src/SessionHistory.jsm @@ +104,5 @@ > * The tab is pinned and should be treated differently for privacy. > + * @param hosts > + * A map containing all hosts of the current docShell that we visited > + * so far. If we encounter a new host while serializing an entry we > + * will add it to the map. What would you think about a naming convention for arguments such as |hosts| that are not outParams but are meant to be modified by side-effect in what is effectively a loop? I don't know of any m-c naming convention for these, but in some other languages, we call these "accumulators", so maybe |accHosts|. @@ +237,5 @@ > + return; > + } > + > + if (/https?/.test(scheme) && !hosts[host] && > + PrivacyLevel.canSave({isHttps: scheme == "https", isPinned: isPinned})) { So, why did you remove the checkPrivacy test?

Could you answer my questions, apply the feedback and then r? me on a merged patch? There are too many patches changing the same files, and with so many moving parts spread across different patches, I have difficulties following it all.

Comment on attachment 802169 [details] [diff] [review] part 1 - Collect hosts per tab when collecting session history Review of attachment 802169 [details] [diff] [review]: ----------------------------------------------------------------- Just one nit, looks good to me. ::: browser/components/sessionstore/src/SessionHistory.jsm @@ +238,5 @@ > + } > + > + if (/https?/.test(scheme) && !hosts[host] && > + PrivacyLevel.canSave({isHttps: scheme == "https", isPinned: isPinned})) { > + // By setting this to true or false, we can determine when looking at Could you maybe be more explicit about what true vs false means semantically? Does true = check for privacy?

Comment on attachment 802211 [details] [diff] [review] part 3 - Store and merge hosts per tab and window, and use that when updating cookies Review of attachment 802211 [details] [diff] [review]: ----------------------------------------------------------------- Other than David's comments, just one nit ::: browser/components/sessionstore/src/SessionStore.jsm @@ +4342,5 @@ > > + // If we're still the latest async collection for the given tab and > + // the caches haven't been filled by collect() in the meantime, let's > + // fill the caches with the data we received. > + let shouldCache = this._pendingCollections.get(tab) == promise; |===| here?

Comment on attachment 802218 [details] [diff] [review] part 5 - Remove code duplication when adding entries to host maps Review of attachment 802218 [details] [diff] [review]: ----------------------------------------------------------------- Nothing to add to what David said.

(In reply to David Rajchenbach Teller [:Yoric] from comment #13) > > + if (/https?/.test(scheme) && !hosts[host] && > > It is not clear to me whether /https?/ is recompiled at each call to the > method. If so, you may wish to precompile it. From https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/RegExp#Special_characters_in_regular_expressions "Use literal notation when the regular expression will remain constant. For example, if you use literal notation to construct a regular expression used in a loop, the regular expression won't be recompiled on each iteration." > > + _extractHostsFromEntry: function (entry, hosts, isPinned) { > > + try { > > + this.addEntryToMap(hosts, makeURI(entry.url), isPinned, false); > > In the original version, |checkPrivacy| was an argument. Now, it is always > |false|. Why? This method is only called from SessionStore._splitCookiesFromWindow() where we never want to check for privacy. All this method does is split cookies that have already been collected, so no need to check again. (In reply to David Rajchenbach Teller [:Yoric] from comment #14) > > + updateForTab: function (tab, hosts) { > > What happens if I modify |hosts| after having called |updateForTab|? That's > certainly not good, could you document this? Fair point, I modified updateForTab to create a copy of the hosts array. Alternatively we could also use Object.freeze()? Not sure if that would be better. > > + mergeTabsToWindow: function (windowID, tabs) { > > I believe that the code would be more robust if we called > |mergeTabsToWindow| behind the scenes, by invalidating the window whenever > we call |updateTab|. This would require passing the |windowID| whenever we > call |updateTab|, of course. I agree this would be a little easier but OTOH I did that so we don't merge hosts from all tabs whenever tab data comes in, which could possibly be very frequently. With the current state of the patch we'd only merge window data max. every 15 secs. Also, when we collect window data the set of tabs might have changed and that would require us to keep track of closing tabs etc. which I'd really like to avoid. (In reply to David Rajchenbach Teller [:Yoric] from comment #15) > > + * @param hosts > > + * A map containing all hosts of the current docShell that we visited > > + * so far. If we encounter a new host while serializing an entry we > > + * will add it to the map. > > What would you think about a naming convention for arguments such as |hosts| > that are not outParams but are meant to be modified by side-effect in what > is effectively a loop? > > I don't know of any m-c naming convention for these, but in some other > languages, we call these "accumulators", so maybe |accHosts|. Sure, I like the idea. (In reply to Steven MacLeod [:smacleod] from comment #17) > > + if (/https?/.test(scheme) && !hosts[host] && > > + PrivacyLevel.canSave({isHttps: scheme == "https", isPinned: isPinned})) { > > + // By setting this to true or false, we can determine when looking at > > Could you maybe be more explicit about what true vs false means > semantically? Does true = check for privacy? Good idea, fixed the comment. That was a little wrong anyway. (In reply to Steven MacLeod [:smacleod] from comment #18) > > + // If we're still the latest async collection for the given tab and > > + // the caches haven't been filled by collect() in the meantime, let's > > + // fill the caches with the data we received. > > + let shouldCache = this._pendingCollections.get(tab) == promise; > > |===| here? When comparing objects (strictly or abstractly) they're always checked for identity so that wouldn't make a difference here and it's more common to use "==" in the code. I do also like expressiveness but decided to stick with the rest of the code base :)

(In reply to Tim Taubert [:ttaubert] from comment #20) > "Use literal notation when the regular expression will remain constant. For > example, if you use literal notation to construct a regular expression used > in a loop, the regular expression won't be recompiled on each iteration." Not 100% that's true inside a function. Still waiting for a clear answer from #jsapi. > This method is only called from SessionStore._splitCookiesFromWindow() where > we never want to check for privacy. All this method does is split cookies > that have already been collected, so no need to check again. Ok. Could you add comments on that |false|, in that case? > > (In reply to David Rajchenbach Teller [:Yoric] from comment #14) > > > + updateForTab: function (tab, hosts) { > > > > What happens if I modify |hosts| after having called |updateForTab|? That's > > certainly not good, could you document this? > > Fair point, I modified updateForTab to create a copy of the hosts array. > Alternatively we could also use Object.freeze()? Not sure if that would be > better. Neither is perfect, but I would go for Object.freeze(), with a comment mentioning that the module takes ownership of the object (whatever that may mean). > > > + mergeTabsToWindow: function (windowID, tabs) { > > > > I believe that the code would be more robust if we called > > |mergeTabsToWindow| behind the scenes, by invalidating the window whenever > > we call |updateTab|. This would require passing the |windowID| whenever we > > call |updateTab|, of course. > > I agree this would be a little easier but OTOH I did that so we don't merge > hosts from all tabs whenever tab data comes in, which could possibly be very > frequently. With the current state of the patch we'd only merge window data > max. every 15 secs. Also, when we collect window data the set of tabs might > have changed and that would require us to keep track of closing tabs etc. > which I'd really like to avoid. Fair point.

(In reply to David Rajchenbach Teller [:Yoric] from comment #21) > (In reply to Tim Taubert [:ttaubert] from comment #20) > > "Use literal notation when the regular expression will remain constant. For > > example, if you use literal notation to construct a regular expression used > > in a loop, the regular expression won't be recompiled on each iteration." > > Not 100% that's true inside a function. Still waiting for a clear answer > from #jsapi. Right. > > This method is only called from SessionStore._splitCookiesFromWindow() where > > we never want to check for privacy. All this method does is split cookies > > that have already been collected, so no need to check again. > > Ok. Could you add comments on that |false|, in that case? Sure. > > Fair point, I modified updateForTab to create a copy of the hosts array. > > Alternatively we could also use Object.freeze()? Not sure if that would be > > better. > > Neither is perfect, but I would go for Object.freeze(), with a comment > mentioning that the module takes ownership of the object (whatever that may > mean). Ok, let's do Object.freeze().

Comment on attachment 808713 [details] [diff] [review] Collect hosts per tab when collecting session history, v3 Review of attachment 808713 [details] [diff] [review]: ----------------------------------------------------------------- ::: browser/components/sessionstore/src/CookieHosts.jsm @@ +27,5 @@ > + * @param string > + * @returns nsIURI > + */ > +function makeURI(aString) { > + return Services.io.newURI(aString, null, null); Food for a followup bug: it might be interesting to consider using DOM object URL (pending bug 920015) instead of a XPCOM URI. @@ +66,5 @@ > +let CookieHostsInternal = { > + /** > + * A weak map storing the list of collected hosts per tab. The keys are > + * tabbrowser tab elements and the values are objects of the form: > + * {"mozilla.org": true, "google.com": false, ...} Remind me: what's the point of this boolean? Is that isPinned? If so, what's the intended semantics when we have a site that appears both as pinned and unpinned? @@ +81,5 @@ > + /** > + * Add the given URI to the given map of hosts if it hasn't been added, yet. > + * > + * @param hosts > + * A map containing all hosts of the current docShell we visited so far. Isn't that a accHosts? @@ +118,5 @@ > + > + /** > + * Replaces the list of collected hosts for a given tab. The given lists of > + * hosts will be frozen to ensure the caller can't modify our internal data > + * structures. We basically take ownership of the passed 'hosts' object. Nit: I would move these two sentences to @param hosts. @@ +133,5 @@ > + }, > + > + /** > + * Merges the per-tab hosts of all tabs currently contained in the given > + * window. You should document why/when this method is meant to be called, e.g. after having updated many tabs, to ensure that future calls to getForWindow will be taken into account. @@ +170,5 @@ > + * > + * @param windowID > + * The ID of the window to retrieve hosts for. > + * @return The list of collected hosts for the given window ID > + * {"mozilla.org": true, "google.com": false, ...} Here, too, please document the boolean. Also, it's a map rather than a list. @@ +189,5 @@ > + > + /** > + * Returns a map of all hosts contained in a given window state object. This > + * is only use by SessionStore._splitCookiesFromWindow() where at startup we > + * might want to split cookies from a given window. Could you explain a little more in detail the difference with |getForWindow|? That one fully sync, isn't it? @@ +223,5 @@ > + * the hash that will be used to store hosts eg, { hostname: true } > + * @param isPinned > + * whether the entry we're evaluating is owned by a pinned tab > + */ > + _extractHostsFromEntry: function (entry, hosts, isPinned) { That one is definitely accHosts. ::: browser/components/sessionstore/src/SessionCookies.jsm @@ +54,2 @@ > */ > update: function (windows) { Could you take the opportunity to document what's changing in |windows| during the course of this call? ::: browser/components/sessionstore/src/SessionHistory.jsm @@ +54,5 @@ > * The docShell that owns the session history. > * @param includePrivateData (optional) > * True to always include private data and skip any privacy checks. > */ > read: function (docShell, includePrivateData = false) { Could you take the opportunity to document the return value? @@ +111,1 @@ > * @return object Could you take the opportunity to detail a little what's in the object? ::: browser/components/sessionstore/src/SessionStore.jsm @@ +2014,5 @@ > } > > // An array that at the end will hold all current window data. > var total = []; > + // A map of window state objects and their window IDs. If it's a map from id to state, please mention it. Otherwise, I don't understand. @@ +2109,1 @@ > _getWindowState: function ssi_getWindowState(aWindow) { Could you take the opportunity to update the documentation of this method? Not that it was correct before your patch, mind you. @@ +2125,3 @@ > }, > > _collectWindowData: function ssi_collectWindowData(aWindow) { Could you take the opportunity to add documentation for this method? @@ +4389,4 @@ > // Put tabData into cache when reading scroll and text data succeeds. > + if (this._updateTextAndScrollDataForTab(tab, tabData) && shouldCache) { > + TabStateCache.set(tab, tabData); > + this._pendingCollections.delete(tab); We use |shouldCache| to determine whether we should update the cookie hosts for the tab. But we do not use it to determine whether we should update the tab state cache. This strikes me as surprising. Even more so since we collect both the tab state data and the cookie data during _collectBaseTabData. Could you explain to me the difference?

This bug is rather critical for performance. Any hope to see it progress?

Sorry, I have a rather huge backlog because we're working on getting the new/old leak detection fixed before I can move on to other work. I'll try to get to it as soon as possible.

Steven, do you think you could take a look at this?

(In reply to David Rajchenbach Teller [:Yoric] <needinfo? me> from comment #27) > Steven, do you think you could take a look at this? This bug may interfere with Bug 930967, so we're holding off until that is fixed.

I realize that bug 930967 is the big one that can collide with everything. However, we should not forget that this bug is the one that impacts users at the moment. I know how much effort has gone into bug 930967, so I won't insist, but let's not schedule other stuff between bug 930967 and this bug.

Collecting "cookie hosts" is quite easy. We can be clever and send updates whenever the list of session history entries changes and maintain a list of hosts in the parent process that is updated every time and have a nice data structure that amortizes access costs. Now the other problem is that this set of hosts per tab isn't saved in sessionstore.js structures and will have to be rebuilt when restoring data, i.e. we would still have to keep this overhead to collect "cookie hosts" from incoming data. A quite easy fix would be to rethink how "cookie hosts" are collected currently. Iterating over all session history entries isn't bad as long as the list is small. With the FrameTree being introduced in bug 921942 we could limit the host collection to the current history entry/frame and its children only. This should cover most of the cases where users crash or restart their browser and would be angry to be logged out of sessions. What do you think about this approach? Best thing about it is that we wouldn't have to change too much about the current cookie collection and get that improvement almost for free.

(In reply to Tim Taubert [:ttaubert] from comment #30) > Collecting "cookie hosts" is quite easy. We can be clever and send updates > whenever the list of session history entries changes and maintain a list of > hosts in the parent process that is updated every time and have a nice data > structure that amortizes access costs. That would be good, yes. > Now the other problem is that this set of hosts per tab isn't saved in > sessionstore.js structures and will have to be rebuilt when restoring data, > i.e. we would still have to keep this overhead to collect "cookie hosts" > from incoming data. Do you mean a one-time-per-session cost during the first collection? I suppose that we could afford it until we can do better. > A quite easy fix would be to rethink how "cookie hosts" are collected > currently. Iterating over all session history entries isn't bad as long as > the list is small. With the FrameTree being introduced in bug 921942 we > could limit the host collection to the current history entry/frame and its > children only. > > This should cover most of the cases where users crash or restart their > browser and would be angry to be logged out of sessions. What do you think > about this approach? Best thing about it is that we wouldn't have to change > too much about the current cookie collection and get that improvement almost > for free. This sounds like a good idea to me. I don't really know why we keep the session cookies for past tabs across sessions.

Was profiling why some canvas code [1] was so janky and see cookie history/extraction showing up in my profiles [2]. 1. http://codepen.io/thebabydino/pen/jgtof 2. http://people.mozilla.org/~bgirard/cleopatra/#report=5beb1729faaaf9f461fbfadde7fb94841004ffd5

Barring any mistake, this bug should now be fixed, shouldn't it?

Collecting cookies should be faster now, yes. Bug 936271 didn't make it into today's Nightly but will be in tomorrow's. Unfortunately just like for DOMSessionStorage (bug 952998) pending tabs won't be recollected as long as you don't select-to-restore them. DOMSessionStorage and session history data will be filtered out once they actually have been loaded. Dietrich, can you please check with tomorrow's Nightly that collecting cookies isn't as costly anymore? To force recollection of all your tabs you could set browser.sessionstore.restore_on_demand=false and then restart and wait a few minutes until all your tabs have loaded, flip the pref back and restart again. That should leave you with freshly collected sessionstore data :)

There's no easy way for me to test this. I have tabs separated across many tab groups. Loading/saving all of them would take many days. I recommend finding some way to measure the impact, and then testing it with a bunch of mock data.

OR change the fix to proactively clean out all of the session data instead of requiring the tab to load.

See http://telemetry.mozilla.org/#nightly/30/FX_SESSION_RESTORE_COLLECT_COOKIES_MS for telemetry on cookie collection.

With the 95th percentile being at a collection time of 15ms (the median being at 3ms) this doesn't seem like a huge deal to me for most of our users. Don't get me wrong, I would really like to have this fixed but without a solution at hand this doesn't seem like the best use of our time currently. The "easiest" way to solve this would probably be to re-think how and why we collect cookies and re-implement this in a manner that is better tested and has much less perf impact. I'm all ears to any ideas but with the current approach we can only get so far, even with complicated caching code.

(In reply to Tim Taubert [:ttaubert] from comment #38) > With the 95th percentile being at a collection time of 15ms (the median > being at 3ms) this doesn't seem like a huge deal to me for most of our > users. Don't get me wrong, I would really like to have this fixed but > without a solution at hand this doesn't seem like the best use of our time > currently. Interestingly, http://telemetry.mozilla.org/#nightly/29/FX_SESSION_RESTORE_COLLECT_DATA_MS has a 95th at 65ms and a 75th around 18ms, (which is not good), while http://telemetry.mozilla.org/#nightly/30/FX_SESSION_RESTORE_COLLECT_DATA_MS has a 95th at 40ms and a 75th at 10ms (which is still not good, but much better). I would really want us to bring down the 95th percentile to ~10ms, because Session Restore is a pretty bad reason to jank. Before deciding on our next course, though, let's wait a few more days until data has stabilized a little.

Numbers are up: http://telemetry.mozilla.org/dist.html#measure=FX_SESSION_RESTORE_COLLECT_COOKIES_MS&max_channel_version=nightly%2F41&min_channel_version=nightly%2F38&product=Firefox&cumulative=0&use_submission_date=0&start_date=2015-05-11&end_date=2015-06-29&sanitize=1 The 95% percentile on Nightly 41 is now 45ms. Plus it was much longer in the profile captured as bug 1182613.

See also http://mzl.la/1K7Z5nr, apparently, it jumped up on 32.

I'm seeing considerable jank from this on a large session. Maybe some loop needs to be broken down into smaller tasks?

After some unrelated freeze causes have been fixed recently this is now the #1 cause for me.

Take this bug since the most noticeable performance bottleneck in bug 1339480 was already identified as this bug, and I am going to list further actions here.

Will, I have a pretty good idea what we can do to fix this, and also a prototype patch that looks promising. Would you mind me taking this?

Hi Tim, it's great! Since you are very familiar with this bug and has a patch, it's great to see you working on this again. Looking forward to your patch, and if you need any assistance such as adding tests while you are busy, please feel free to tell me, thanks :)

(In reply to Will Wang [:WillWang] from comment #50) > Looking forward to your patch, and if you need any assistance such as adding > tests while you are busy, > please feel free to tell me, thanks :) I will, thank you!

Ok, here's a patch that fixes the main process jank by slightly reworking how we deal with session cookies, hopefully avoiding other annoyances too. For details, please see the commit message. I removed browser_cookies.js as that was basically only testing the filtering that this patch removes. I added two new tests that are faster and test the new behavior. I would have loved to add more tests but there's some stuff we just can't test without a framework that supports shutdown/restart. The deferred restore I could test only by hand. The net benefit is much simpler code, and cookie serialization that does almost no work when collecting sessionstore data.

Small test fix.

Well, Mike Conley did add http://searchfox.org/mozilla-central/source/testing/firefox-ui/tests/functional/sessionstore/test_restore_windows_after_restart.py not too long ago, which might be a good blueprint for what you're trying to test here?

(In reply to Mike de Boer [:mikedeboer] from comment #54) > Well, Mike Conley did add > http://searchfox.org/mozilla-central/source/testing/firefox-ui/tests/ > functional/sessionstore/test_restore_windows_after_restart.py not too long > ago, which might be a good blueprint for what you're trying to test here? Interesting, I'll take a look!

(In reply to Tim Taubert [:ttaubert] from comment #52) > Ok, here's a patch that fixes the main process jank by slightly reworking > how we deal with session cookies, hopefully avoiding other annoyances too. > For details, please see the commit message. FTR, there's also a lot of discussion in bug 530594, people over there are probably not happy having more cookies restored. But then they haven't had much more control/insight over what we store before. We still respect the privacy_level pref.

Comment on attachment 8854403 [details] [diff] [review] 0001-Bug-912717-Don-t-let-SessionCookie-collection-jank-t.patch, v2 Review of attachment 8854403 [details] [diff] [review]: ----------------------------------------------------------------- Re bug bug 530594: I don't think it's *a lot* of discussion, but yeah there is talk about how we can better provide privacy control levers to our users. This is something we need to start fixing in the UI, once shorlander starts answering needinfo's. But this patch is looking awesome! I'm really happy with the simplification. I do have a few questions, though, here and there. But nothing big, I think. Looking forward to the next iteration! ::: browser/components/sessionstore/SessionCookies.jsm @@ +241,2 @@ > */ > + serialize() { nit: this is not really serializing as we generally busy the term. How about `toArray()`, instead? Or if you like it more generic: `getAll()`? @@ +245,5 @@ > > + /** > + * Returns the key needed to properly store and identify a given cookie. > + * A cookie is uniquely identified by the combination of its host, name, > + * path, and originAttributes. nit: add ' properties' at the end of this sentence and please complete the doc here with @param and @return descriptions. @@ +252,5 @@ > + return JSON.stringify({ > + host: cookie.host, > + name: cookie.name, > + path: cookie.path, > + attr: ChromeUtils.originAttributesToSuffix(cookie.originAttributes) Isn't `expiry` supposed to be one of the deterministic properties? Or is this where I ought to understand that session cookies never expire? ::: browser/components/sessionstore/SessionStore.jsm @@ +635,5 @@ > // If we have a iniState with windows, that means that we have windows > // with app tabs to restore. > + if (iniState.windows.length) { > + iniState.cookies = remainingState.cookies; > + delete remainingState.cookies; You're saying this is converting the old storage format to the new one, right? Can you mention that briefly in a comment here? @@ +3111,5 @@ > > + // Collect and store session cookies. > + TelemetryStopwatch.start("FX_SESSION_RESTORE_COLLECT_COOKIES_MS"); > + state.cookies = SessionCookies.collect(); > + TelemetryStopwatch.finish("FX_SESSION_RESTORE_COLLECT_COOKIES_MS"); Looking forward to seeing the new graphs ;-) @@ +3347,5 @@ > this.restoreWindowFeatures(aWindow, winData); > delete this._windows[aWindow.__SSi].extData; > } > + > + // Restore cookies from legacy sessions. nit: maybe add ' from before bug 912717.'? ::: browser/components/sessionstore/test/browser_cookies_legacy.js @@ +25,5 @@ > +// Test that calling ss.setBrowserState() with a legacy state object that > +// contains cookies in the window state restores session cookies properly. > +add_task(function* test_browser() { > + let [state, cookie] = createTestState(); > + yield Promise.all([waitForNewCookie(cookie), promiseBrowserState(state)]); What I've been thinking about lately: We have intermittents starting when the order of sessionstore test file execution in a suite changes. In these tests we do *poof* new browser state a lot, but never reset it in the profile to the original state, even though we _always_ neatly clean up windows and tabs we created. Wouldn't it be appropriate to blast the original state where it belongs for good measure? That what I saw a contributor (GSoC applicant) do recently and I thought it made sense. What do you think? @@ +35,5 @@ > + let cookie = { > + host: "http://example.com", > + path: "/", > + name: `name${r}`, > + value: `value${r}` If 'expiry' should be here, can you add it? ::: browser/components/sessionstore/test/browser_cookies_privacy.js @@ +74,5 @@ > + ok(addCookie("https", true) && !getCookie(), "secure https cookie not stored"); > + Services.cookies.removeAll(); > +}); > + > +function addCookie(scheme, secure = false) { nit: I'm used to seeing all the helper things defined at the top of the file (together with necessary imports, constants and the like). This has the added benefit that I can easily scan for stuff that's undefined or supposed to be found in head.js. Your choice!

(In reply to Mike de Boer [:mikedeboer] from comment #57) > ::: browser/components/sessionstore/SessionCookies.jsm > > + serialize() { > > nit: this is not really serializing as we generally busy the term. How about > `toArray()`, instead? Or if you like it more generic: `getAll()`? toArray() sgtm. > > + return JSON.stringify({ > > + host: cookie.host, > > + name: cookie.name, > > + path: cookie.path, > > + attr: ChromeUtils.originAttributesToSuffix(cookie.originAttributes) > > Isn't `expiry` supposed to be one of the deterministic properties? Or is > this where I ought to understand that session cookies never expire? Talked about this on IRC, `expiry` isn't supposed to be in there. Should be unset for session cookies anyway afaik. > ::: browser/components/sessionstore/SessionStore.jsm > > // If we have a iniState with windows, that means that we have windows > > // with app tabs to restore. > > + if (iniState.windows.length) { > > + iniState.cookies = remainingState.cookies; > > + delete remainingState.cookies; > > You're saying this is converting the old storage format to the new one, > right? Can you mention that briefly in a comment here? This is actually moving the cookies over so that they're restored properly for deferred sessions. I'll add a comment. > > + // Collect and store session cookies. > > + TelemetryStopwatch.start("FX_SESSION_RESTORE_COLLECT_COOKIES_MS"); > > + state.cookies = SessionCookies.collect(); > > + TelemetryStopwatch.finish("FX_SESSION_RESTORE_COLLECT_COOKIES_MS"); > > Looking forward to seeing the new graphs ;-) Me too. I hope they'll be binary, i.e. between 0 and 1 ;) > ::: browser/components/sessionstore/test/browser_cookies_legacy.js > @@ +25,5 @@ > > +// Test that calling ss.setBrowserState() with a legacy state object that > > +// contains cookies in the window state restores session cookies properly. > > +add_task(function* test_browser() { > > + let [state, cookie] = createTestState(); > > + yield Promise.all([waitForNewCookie(cookie), promiseBrowserState(state)]); > > What I've been thinking about lately: > > We have intermittents starting when the order of sessionstore test file > execution in a suite changes. In these tests we do *poof* new browser state > a lot, but never reset it in the profile to the original state, even though > we _always_ neatly clean up windows and tabs we created. Wouldn't it be > appropriate to blast the original state where it belongs for good measure? > That what I saw a contributor (GSoC applicant) do recently and I thought it > made sense. What do you think? Yeah, I've done that before in other tests... Probably a good idea, done.

I started looking into firefox-ui tests but the framework is missing a few things (like pinning tabs) and writing/reading cookies seems to be quite rudimentary still. I'd love to finish the tests I started but especially with framework modifications we should probably work on those in another bug?

(In reply to Tim Taubert [:ttaubert] (on PTO, back April 24th) from comment #59) > I started looking into firefox-ui tests but the framework is missing a few > things (like pinning tabs) and writing/reading cookies seems to be quite > rudimentary still. I'd love to finish the tests I started but especially > with framework modifications we should probably work on those in another bug? I had expected this, so was already prepared to see that happening at a later time in a different bug, by someone else perhaps even. Can you file it, for posterity, please?

Comment on attachment 8855315 [details] [diff] [review] 0001-Bug-912717-Don-t-let-SessionCookie-collection-jank-t.patch, v3 Review of attachment 8855315 [details] [diff] [review]: ----------------------------------------------------------------- I'm proud of you - this turned out to be beautiful code. Is that something I can say? Oh well, damage = done :-) ::: browser/components/sessionstore/SessionCookies.jsm @@ +35,5 @@ > > /** > * The internal API. > */ > +let SessionCookiesInternal = { Is `let` in the module-global scope not costly anymore? I think you should change this back to `var`. @@ +184,2 @@ > */ > +let CookieStore = { Same here ;-) @@ +260,2 @@ > */ > + _keyForCookie(cookie) { nit: please add the operand in the name - perhaps that's more muscle memory (for the lack of a better psychology jargon) than anything, but `_getKeyForCookie` just sounds less gangsta to me.

(In reply to Mike de Boer [:mikedeboer] from comment #61) > > +let SessionCookiesInternal = { > > Is `let` in the module-global scope not costly anymore? I think you should > change this back to `var`. I had no clue about this. Will revert both. > @@ +260,2 @@ > > */ > > + _keyForCookie(cookie) { > > nit: please add the operand in the name - perhaps that's more muscle memory > (for the lack of a better psychology jargon) than anything, but > `_getKeyForCookie` just sounds less gangsta to me. Sure, will do.

(In reply to Mike de Boer [:mikedeboer] from comment #60) > (In reply to Tim Taubert [:ttaubert] (on PTO, back April 24th) from comment > #59) > > I started looking into firefox-ui tests but the framework is missing a few > > things (like pinning tabs) and writing/reading cookies seems to be quite > > rudimentary still. I'd love to finish the tests I started but especially > > with framework modifications we should probably work on those in another bug? > > I had expected this, so was already prepared to see that happening at a > later time in a different bug, by someone else perhaps even. > > Can you file it, for posterity, please? Of course. Filed bug 1354523.

Pushed by ttaubert@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/93d54f86f07e Don't let SessionCookie collection jank the chrome process r=mikedeboer

Pushed by ttaubert@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/3dd31cc99bb3 Tiny follow-up to fix ESLint bustage caused by a test file r=bustage

(In reply to The 8472 from comment #46) > After some unrelated freeze causes have been fixed recently this is now the > #1 cause for me. If you find the time, any feedback on the impact of this patch would be highly appreciated. SessionCookies.jsm should hopefully not interfere anymore. The startup time might also be a second shorter as it basically also fixes bug 1353533 (again).

https://hg.mozilla.org/mozilla-central/rev/93d54f86f07e https://hg.mozilla.org/mozilla-central/rev/3dd31cc99bb3

(In reply to Tim Taubert [:ttaubert] (on PTO, back April 24th) from comment #66) > (In reply to The 8472 from comment #46) > > After some unrelated freeze causes have been fixed recently this is now the > > #1 cause for me. > > If you find the time, any feedback on the impact of this patch would be > highly appreciated. SessionCookies.jsm should hopefully not interfere > anymore. The startup time might also be a second shorter as it basically > also fixes bug 1353533 (again). Nightly with the profiler enabled is currently to crashy to get a long enough uptime to tell.

Tim, thanks a lot for fixing this. :-) You can get data about this on the Nightly channel from the Background Hang Reports data from telemetry. I have seen this in the BHR data before. In order to see it in mconley's daily snapshots <https://s3-us-west-2.amazonaws.com/telemetry-public-analysis-2/bhr-stacks/data/snapshot-daily.zip> (from bug 1344003 comment 20) in the parent json file. You can search for SessionCookies.jsm there and read through the data. :-) You can also use the analysis.telemetry.mozilla.org infrastructure to query this data set manually. mconley can show you an example ipython notebook which dumps out this daily snapshot zip file.

The telemetry evolution dashboard shows a nice line: https://telemetry.mozilla.org/new-pipeline/evo.html#!aggregates=median&cumulative=0&end_date=null&keys=&max_channel_version=nightly%252F55&measure=FX_SESSION_RESTORE_COLLECT_COOKIES_MS&min_channel_version=nightly%252F52&processType=*&product=Firefox&sanitize=1&sort_keys=submissions&start_date=null&trim=1&use_submission_date=0 Flat enough that we could probably remove the telemetry probe in the near future. (In reply to :Ehsan Akhgari (super long backlog, slow to respond, not reviewing for a while) from comment #69) > You can get data about this on the Nightly channel from the Background Hang > Reports data from telemetry. I have seen this in the BHR data before. In > order to see it in mconley's daily snapshots > <https://s3-us-west-2.amazonaws.com/telemetry-public-analysis-2/bhr-stacks/ > data/snapshot-daily.zip> (from bug 1344003 comment 20) in the parent json > file. You can search for SessionCookies.jsm there and read through the > data. :-) I just downloaded the snapshot and found a few occurrences of SessionCookies.jsm. Some seem to stem from a cookie manager add-on. The other ones have "js::GCRuntime::collect" at the bottom. Most of the latter ones have line numbers that seem to indicate an older version of the file though, so maybe we just have to wait a little for the update to reach more users?

FTR, COLLECT_DATA_MS went down nicely as well: https://telemetry.mozilla.org/new-pipeline/evo.html#!aggregates=median&cumulative=0&end_date=null&keys=&max_channel_version=nightly%252F55&measure=FX_SESSION_RESTORE_COLLECT_DATA_MS&min_channel_version=nightly%252F52&processType=*&product=Firefox&sanitize=1&sort_keys=submissions&start_date=null&trim=1&use_submission_date=0

(In reply to Tim Taubert [:ttaubert] from comment #71) > FTR, COLLECT_DATA_MS went down nicely as well: > > https://telemetry.mozilla.org/new-pipeline/evo.html#! > aggregates=median&cumulative=0&end_date=null&keys=&max_channel_version=nightl > y%252F55&measure=FX_SESSION_RESTORE_COLLECT_DATA_MS&min_channel_version=night > ly%252F52&processType=*&product=Firefox&sanitize=1&sort_keys=submissions&star > t_date=null&trim=1&use_submission_date=0 Very nice! Here is a fancier version of this chart, FWIW: <https://health.graphics/quantum/track?metric=FX_SESSION_RESTORE_COLLECT_DATA_MS>. It seems like FX_SESSION_RESTORE_COLLECT_COOKIES_MS is already removed by now.

(In reply to :Ehsan Akhgari (super long backlog, slow to respond) from comment #72) > Very nice! Here is a fancier version of this chart, FWIW: > <https://health.graphics/quantum/ > track?metric=FX_SESSION_RESTORE_COLLECT_DATA_MS>. It seems like > FX_SESSION_RESTORE_COLLECT_COOKIES_MS is already removed by now. Nice, that's fancier indeed. I'll need to add some more telemetry to find out what we're spending the rest of the time on.

(In reply to Tim Taubert [:ttaubert] from comment #66) > (In reply to The 8472 from comment #46) > > After some unrelated freeze causes have been fixed recently this is now the > > #1 cause for me. > > If you find the time, any feedback on the impact of this patch would be > highly appreciated. SessionCookies.jsm should hopefully not interfere > anymore. The startup time might also be a second shorter as it basically > also fixes bug 1353533 (again). Well, the profiler is behaving better now and I and none of the (much less common now!) hangs could be attributed to that JSM. So it looks good from here.