812379 - Expose taburl to profiler in sessionstore's ssi_collectTabData()

Status: NEW

(Firefox :: Session Restore, defect)

Description

[(dormant account)]

This will make it easier to identify bad tabs in the profiler traces

Comment 1

[:Irving Reid (No longer working on Firefox)]

Attachment: First cut at hook to record tab URL in the profile

This uses eval() to create a new function that contains a sanitized version of the URL, so that the URL is reasonably visible on the stack without needing to expose any new APIs.

I structured it like a monkeypatch to leave us the option of turning it on and off at run time.

Comment 2

[Benoit Girard (:BenWa)]

Comment on attachment 684901 [details] [diff] [review]
First cut at hook to record tab URL in the profile

Review of attachment 684901 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. There's still a dump in here.

Perhaps you should check profiler.IsActive() and only eval then to be as least invasive to release configuration.

Comment 3

[(dormant account)]

Comment on attachment 684901 [details] [diff] [review]
First cut at hook to record tab URL in the profile

I agree with Benoit's nits.
 Also this regexp is overly ugly and destructive. Why not do this via a safer(and possibly faster?) base64? Then it could be properly decoded in the profiler. Do we expose a fast base64 api somewhere?

Comment 4

[(dormant account)]

Comment on attachment 684901 [details] [diff] [review]
First cut at hook to record tab URL in the profile

window.btoa is the API I was thinking of.

Comment 5

[Benoit Girard (:BenWa)]

window.btoa wont work in function name because function names in JS can't include '/' character '=' character. We will need a more restricted encoding.

Comment 6

[:Irving Reid (No longer working on Firefox)]

Attachment: Create a JS method on the fly to include tab URL in profile call tree

Cleaned up a bit, and this version uses the profiler-started and profiler-stopped notifications to insert and remove the profile wrapper from the SessionStore object.

I didn't change the way I sanitize the URL, because all readily available encoding formats still don't produce legal JS identifiers and I think our effort would be better spent on a different approach to tagging methods in profiler traces.

My suggestion would be to approach the JS team about adding support for a custom attribute on JS functions (Function._profile_tag, perhaps) and have that appended to (or pushed in place of) the function name on the profile stack

Comment 7

[Benoit Girard (:BenWa)]

Comment on attachment 688902 [details] [diff] [review]
Create a JS method on the fly to include tab URL in profile call tree

Review of attachment 688902 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/sessionstore/src/SessionStore.jsm
@@ +339,5 @@
>  #endif
>  
> +    this._profilerService = null;
> +    try {
> +      this._profilerService = Cc["@mozilla.org/tools/profiler;1"].getService(Ci.nsIProfiler);

I am fine with this as long as fetching the service here is cheap.

Comment 8

[:Irving Reid (No longer working on Firefox)]

Comment on attachment 688902 [details] [diff] [review]
Create a JS method on the fly to include tab URL in profile call tree

Sorry about the mis-routed f?

Comment 9

[Vladan Djeric (:vladan)]

Comment on attachment 688902 [details] [diff] [review]
Create a JS method on the fly to include tab URL in profile call tree

After a lengthy discussion with Ehsan, I've also started wondering if there is a benefit to including the tab URL in the profile. The page currently loaded in the tab is likely only of many things that could be slowing down tabCollectData, e.g. long back/forward history, fragmented sessionStore history file, busy disk, etc. It might be very misleading to report the tab URL in shutdown profiles and it could send developers on wild-goose chases. Perhaps this patch is better suited for local builds of developers investigating shutdown times.
Do we currently have any URLs which are known to reliably slow down tabCollectData?

>   /**
>+   * Profiling wrapper for _collectTabData
>+   * Generates a temporary function with the URL encoded in its name so the profile
>+   * can identify particular tabs for which _collectTabData is slow
>+   * @param aTab
>+   *        tabbrowser tab
>+   * @param aFullData
>+   *        always return privacy sensitive data (use with care)
>+   * @returns object
>+   */
>+  _profile_collectTabData: function ssi_profile_collectTabData(aTab, aFullData) {

Nit: the @returns comment should say something about the object being returned

>+    if (browser && browser.currentURI) {
>+      var name = browser.currentURI.spec
>+                   .replace(/[.:]/g, "_")
>+                   .replace(/[/#?=,]/g, "$")
>+                   .replace(/[^a-zA-Z0-9_$]/g, "x");

This makes me uncomfortable.. we're evaluating a page URI (untrusted) in a privileged context. If there is a bug in the regex code, this could become an sg:crit. Can we push a dynamic label somehow or use your Function._profile_tag solution?

Comment 10

[(no longer active)]

Also, compiling three regex's on every call to this function seems quite expensive, especially since in many cases the entire thing can be so cheap that this can skew the results.

Comment 11

[Benoit Girard (:BenWa)]

(In reply to Vladan Djeric (:vladan) from comment #9)
> Comment on attachment 688902 [details] [diff] [review]
> Create a JS method on the fly to include tab URL in profile call tree
> 
> After a lengthy discussion with Ehsan, I've also started wondering if there
> is a benefit to including the tab URL in the profile. The page currently
> loaded in the tab is likely only of many things that could be slowing down
> tabCollectData, e.g. long back/forward history, fragmented sessionStore
> history file, busy disk, etc. It might be very misleading to report the tab
> URL in shutdown profiles and it could send developers on wild-goose chases.
> Perhaps this patch is better suited for local builds of developers
> investigating shutdown times.
> Do we currently have any URLs which are known to reliably slow down
> tabCollectData?
> 

I don't agree with this. There's already a lot of data that is misleading in a profile. (Is the problem in the leaf string cat function or is the caller asking to concat 1GB of text). I think including the URL just adds more data. If a profile is because a tab is slow load we want to know which one. I could be convince after this lands that we want to hide this information by default but I don't think we should be shy of collecting data that doesn't incur overhead and instead prefer hiding it by default.

We could wait on Function._profile_tag if we don't think this is particularly high priority.

Comment 12

[(no longer active)]

(In reply to comment #11)
> I don't agree with this. There's already a lot of data that is misleading in a
> profile. (Is the problem in the leaf string cat function or is the caller
> asking to concat 1GB of text). I think including the URL just adds more data.
> If a profile is because a tab is slow load we want to know which one. I could
> be convince after this lands that we want to hide this information by default
> but I don't think we should be shy of collecting data that doesn't incur
> overhead and instead prefer hiding it by default.

The point is that _collectTabData doesn't actually have anything to do with loading a tab.  It is the function that looks at a given tab and collects information about it.  Here's a rough list of the data that it looks at:

* the tab's URI
* whether the tab is pinned (is an app tab)
* the tab's session history entries
* what the user has typed in the location bar when that tab had focus
* whether the tab is hidden
* the tab's docshell permissions
* the tab's DOM attribute (the attributes of the XUL <tab> element)
* the tab's session storage

Almost none of this data except for perhaps the last one has any relevance to the URL of the document loaded in the tab.  For example, things such as the number of session history entries can have a much more serious impact on the performance of this function.  By reporting the current URL of the tab and not any of the other factors, we will be discounting those factors, and this will lead to misleading results and evaluations, or to developers wasting their time at best.

I don't buy your argument that collecting more data is always valuable.  It can actually be quite misleading if the data you collect corresponds to a very small percentage of the performance implications of the code in question, and all other factors are left unmeasured.

Comment 13

[Vladan Djeric (:vladan)]

So in light of the arguments put forth above, I'd be ok with closing this bug or postponing further work until we have evidence these URIs would be useful & not very misleading