Closed
Bug 701583
Opened 13 years ago
Closed 13 years ago
make a snapshot of sqlite data on startup
Categories
(Toolkit :: Telemetry, defect)
Tracking
()
RESOLVED
FIXED
mozilla11
People
(Reporter: taras.mozilla, Assigned: taras.mozilla)
References
Details
(Whiteboard: [inbound])
Attachments
(1 file, 3 obsolete files)
5.46 KB,
patch
|
mak
:
review+
|
Details | Diff | Splinter Review |
No description provided.
Assignee | ||
Comment 1•13 years ago
|
||
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 2•13 years ago
|
||
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 3•13 years ago
|
||
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•13 years ago
|
||
Assignee: nobody → tglek
Attachment #573703 -
Attachment is obsolete: true
Attachment #573703 -
Flags: review?(mak77)
Attachment #573884 -
Flags: review?(mak77)
Assignee | ||
Comment 5•13 years ago
|
||
Attachment #573884 -
Attachment is obsolete: true
Attachment #573884 -
Flags: review?(mak77)
Attachment #573890 -
Flags: review?(mak77)
Assignee | ||
Comment 6•13 years ago
|
||
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•13 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 8•13 years ago
|
||
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+
Comment 9•13 years ago
|
||
(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.
Comment 10•13 years ago
|
||
that said, don't think you should change to weak references in this bug, was just a side note :)
Assignee | ||
Comment 11•13 years ago
|
||
http://hg.mozilla.org/integration/mozilla-inbound/rev/7f798afba1da
Whiteboard: [inbound]
Comment 12•13 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/7f798afba1da
Status: NEW → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla11
You need to log in
before you can comment on or make changes to this bug.
Description
•