make a snapshot of sqlite data on startup

RESOLVED FIXED in mozilla11

Status

()

Toolkit
Telemetry
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: (dormant account), Assigned: (dormant account))

Tracking

unspecified
mozilla11
x86
Windows 7
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [inbound])

Attachments

(1 attachment, 3 obsolete attachments)

Comment hidden (empty)
(Assignee)

Comment 1

6 years ago
Created attachment 573703 [details] [diff] [review]
make an sqlite io snapshot on startup

This is a hack. The proper way to do this is to add a clone() method to histograms, then about:telemetry will be able to show this. However I'd like to get this in ASAP so we can debug the extend of sqlite overhead on startup
Attachment #573703 - Flags: review?(mak77)
Comment on attachment 573703 [details] [diff] [review]
make an sqlite io snapshot on startup

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

::: toolkit/components/telemetry/TelemetryPing.js
@@ +124,4 @@
>    _histograms: {},
>    _initialized: false,
>    _prevValues: {},
> +  _sqliteOverhead: null,

why null and not an empty object, in getHistograms you walk it without checking if it's null. Surely you expect gatherStartupSqlite to be invoked before, but you can't tell from here.

@@ +137,5 @@
> +    let hls = Telemetry.histogramSnapshots;
> +    let ret = {};
> +    for (let key in this._sqliteOverhead) {
> +      hls[key] = this._sqliteOverhead[key]
> +    }

newline before the loop, and a nice comment explaining what happens here would be welcome! And comments are free :)

@@ +305,5 @@
> +    for (let key in hls) {
> +      if (key.search("SQLITE"))
> +        sqliteOverhead["STARTUP_" + key] = hls[key];
> +    }
> +    this._sqliteOverhead = sqliteOverhead;

if you make _sqliteOverhead an object from the beginning, you don't need a temporary object here, just put stuff into it.

@@ +394,4 @@
>      }
>      Services.obs.addObserver(this, "private-browsing", false);
>      Services.obs.addObserver(this, "profile-before-change", false);
> +    Services.obs.addObserver(this, "sessionstore-windows-restored", false);

is this late enough? I thought from a previous discussion this was firing early, for example while windows have been restored their tabs have not, so this would not capture eventual DOMStorage stuff in the homepage.

@@ +419,2 @@
>      Services.obs.removeObserver(this, "profile-before-change");
>      Services.obs.removeObserver(this, "private-browsing");

OT: if telemetryPing is a service, and looks like, you may implement nsIWeakReference and use weak observers.
Comment on attachment 573703 [details] [diff] [review]
make an sqlite io snapshot on startup

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

::: toolkit/components/telemetry/TelemetryPing.js
@@ +302,5 @@
> +    let hls = Telemetry.histogramSnapshots;
> +    let sqliteOverhead = {};
> +    
> +    for (let key in hls) {
> +      if (key.search("SQLITE"))

use a match here, it's faster: if (/SQLITE/.test(key))
(Assignee)

Comment 4

6 years ago
Created attachment 573884 [details] [diff] [review]
v2
Assignee: nobody → tglek
Attachment #573703 - Attachment is obsolete: true
Attachment #573703 - Flags: review?(mak77)
Attachment #573884 - Flags: review?(mak77)
(Assignee)

Comment 5

6 years ago
Created attachment 573890 [details] [diff] [review]
v3
Attachment #573884 - Attachment is obsolete: true
Attachment #573884 - Flags: review?(mak77)
Attachment #573890 - Flags: review?(mak77)
(Assignee)

Comment 6

6 years ago
Created attachment 573891 [details] [diff] [review]
v4

sorry for bugsspam, forgot to qref
Attachment #573890 - Attachment is obsolete: true
Attachment #573890 - Flags: review?(mak77)
Attachment #573891 - Flags: review?(mak77)
(Assignee)

Comment 7

6 years ago
> @@ +394,4 @@
> >      }
> >      Services.obs.addObserver(this, "private-browsing", false);
> >      Services.obs.addObserver(this, "profile-before-change", false);
> > +    Services.obs.addObserver(this, "sessionstore-windows-restored", false);
> 
> is this late enough? I thought from a previous discussion this was firing
> early, for example while windows have been restored their tabs have not, so
> this would not capture eventual DOMStorage stuff in the homepage.

session-restored is pretty well correlated with firstpaint. So while it is not perfect, it'll do for now. 
> 
> @@ +419,2 @@
> >      Services.obs.removeObserver(this, "profile-before-change");
> >      Services.obs.removeObserver(this, "private-browsing");
> 
> OT: if telemetryPing is a service, and looks like, you may implement
> nsIWeakReference and use weak observers.

I don't know anything about this.
Comment on attachment 573891 [details] [diff] [review]
v4

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

::: toolkit/components/telemetry/TelemetryPing.js
@@ +136,5 @@
> +  getHistograms: function getHistograms() {
> +    let hls = Telemetry.histogramSnapshots;
> +    let ret = {};
> +
> +    // bug 701583: report sqlite overhead on startup

I would have preferred a couple descriptive lines, but I'll survive this

@@ +138,5 @@
> +    let ret = {};
> +
> +    // bug 701583: report sqlite overhead on startup
> +    for (let key in this._sqliteOverhead) {
> +      hls[key] = this._sqliteOverhead[key]

missing semicolon at the end

@@ +299,4 @@
>      }
>    },
>    
> +  /** Make a copy of sqlite histograms on startup */

Make this a proper javadoc, kthx!
Attachment #573891 - Flags: review?(mak77) → review+
(In reply to Taras Glek (:taras) from comment #7)
> > OT: if telemetryPing is a service, and looks like, you may implement
> > nsIWeakReference and use weak observers.
> 
> I don't know anything about this.

practically you do addObserver(..., false), and later removeObserver(...), if you implement weak references you just do addObserver(..., true) and you don't care to remove. Since this is service it's kept alive by the service manager and does not need a strong reference by the observer service.
that said, don't think you should change to weak references in this bug, was just a side note :)
(Assignee)

Updated

6 years ago
Blocks: 701863
https://hg.mozilla.org/mozilla-central/rev/7f798afba1da
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla11
You need to log in before you can comment on or make changes to this bug.