Closed Bug 701583 Opened 8 years ago Closed 8 years ago

make a snapshot of sqlite data on startup

Categories

(Toolkit :: Telemetry, defect)

x86
Windows 7
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla11

People

(Reporter: taras.mozilla, Assigned: taras.mozilla)

References

Details

(Whiteboard: [inbound])

Attachments

(1 file, 3 obsolete files)

No description provided.
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))
Attached patch v2 (obsolete) — Splinter Review
Assignee: nobody → tglek
Attachment #573703 - Attachment is obsolete: true
Attachment #573703 - Flags: review?(mak77)
Attachment #573884 - Flags: review?(mak77)
Attached patch v3 (obsolete) — Splinter Review
Attachment #573884 - Attachment is obsolete: true
Attachment #573884 - Flags: review?(mak77)
Attachment #573890 - Flags: review?(mak77)
Attached patch v4Splinter Review
sorry for bugsspam, forgot to qref
Attachment #573890 - Attachment is obsolete: true
Attachment #573890 - Flags: review?(mak77)
Attachment #573891 - Flags: review?(mak77)
> @@ +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 :)
Blocks: 701863
https://hg.mozilla.org/mozilla-central/rev/7f798afba1da
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla11
You need to log in before you can comment on or make changes to this bug.