Closed
Bug 675737
Opened 13 years ago
Closed 13 years ago
Calculate time taken for by write/open/sync per db + main/other thread
Categories
(Toolkit :: Storage, defect)
Toolkit
Storage
Tracking
()
RESOLVED
FIXED
mozilla8
People
(Reporter: taras.mozilla, Assigned: taras.mozilla)
References
Details
(Whiteboard: [inbound])
Attachments
(1 file, 2 obsolete files)
(deleted),
patch
|
mak
:
review+
|
Details | Diff | Splinter Review |
Tracking io based on file + thread results in a slight combinatorial explosion that's papered over with macros.
Attachment #549922 -
Flags: review?(mak77)
Updated•13 years ago
|
Assignee: nobody → tglek
Status: NEW → ASSIGNED
Updated•13 years ago
|
OS: Linux → All
Hardware: x86 → All
Comment 1•13 years ago
|
||
Comment on attachment 549922 [details] [diff] [review] more precise accounting Review of attachment 549922 [details] [diff] [review]: ----------------------------------------------------------------- A side note: you may have to rebase your patch manually, automatic merges in TelemetryHistograms.h make probes randomly interleaved (you may end up having Places probes in the middle of network ones if you trust rebase). I'd still like to take a quick look at final version with these comments addressed, so f+ for now. ::: storage/src/TelemetryVFS.cpp @@ +77,5 @@ > class IOThreadAutoTimer { > public: > + /** > + * @param id is MOZ_SQLITE_OPEN_MS or any id that is immediately followed by > + * _MAIN_THREAD_MS equivalent Please expand this comment up to an obvious point, it took a while to figure out how you were matching on the MAIN_THREAD_MS versions of the probes @@ +87,4 @@ > } > > ~IOThreadAutoTimer() { > + Telemetry::Accumulate(static_cast<Telemetry::ID>(id + NS_IsMainThread()), here you are implicitly using the fact NS_IsMainThread returns a PRBool, and using that to increment as if it was an int (it is after all). This is not sane in the current try to move to real bools, you should handle it as if it was a bool, a ternary operator may be fine. ::: toolkit/components/telemetry/TelemetryHistograms.h @@ +119,5 @@ > HISTOGRAM(FIND_PLUGINS, 1, 3000, 10, EXPONENTIAL, "Time spent scanning filesystem for plugins (ms)") > HISTOGRAM(CHECK_JAVA_ENABLED, 1, 3000, 10, EXPONENTIAL, "Time spent checking if Java is enabled (ms)") > +#define SQLITE_TIME_SPENT(NAME, DESC) \ > +HISTOGRAM(MOZ_SQLITE_ ## NAME ## _MS, 1, 3000, 10, EXPONENTIAL, DESC) \ > +HISTOGRAM(MOZ_SQLITE_ ## NAME ## _MAIN_THREAD_MS, 1, 3000, 10, EXPONENTIAL, DESC) could you indent once here? add a note about the fact order and adjacency of these probes matters for correct functionality. @@ +121,5 @@ > +#define SQLITE_TIME_SPENT(NAME, DESC) \ > +HISTOGRAM(MOZ_SQLITE_ ## NAME ## _MS, 1, 3000, 10, EXPONENTIAL, DESC) \ > +HISTOGRAM(MOZ_SQLITE_ ## NAME ## _MAIN_THREAD_MS, 1, 3000, 10, EXPONENTIAL, DESC) > + > +#define SQLITE_MULTIPLEX(NAME, DESC) \ SQLITE_TIME_SPENT_MULTIPLEX would be more self-documenting, mutiplex doesn't tell me much about what it measures @@ +125,5 @@ > +#define SQLITE_MULTIPLEX(NAME, DESC) \ > + SQLITE_TIME_SPENT(OTHER_ ## NAME, DESC) \ > + SQLITE_TIME_SPENT(PLACES_ ## NAME, DESC) \ > + SQLITE_TIME_SPENT(COOKIES_ ## NAME, DESC) \ > + SQLITE_TIME_SPENT(URLCLASSIFIER_ ## NAME, DESC) I dunno where the description is used (if it is at all) in telemetry, maybe you may prefix DESC with "Other: ", "Places: ", "Cookies: ", ...
Attachment #549922 -
Flags: review?(mak77) → feedback+
Assignee | ||
Comment 2•13 years ago
|
||
(In reply to comment #1) > A side note: you may have to rebase your patch manually, automatic merges in > TelemetryHistograms.h make probes randomly interleaved (you may end up > having Places probes in the middle of network ones if you trust rebase). I never trust rebase :) > > @@ +87,4 @@ > > } > > > > ~IOThreadAutoTimer() { > > + Telemetry::Accumulate(static_cast<Telemetry::ID>(id + NS_IsMainThread()), > > here you are implicitly using the fact NS_IsMainThread returns a PRBool, and > using that to increment as if it was an int (it is after all). This is not > sane in the current try to move to real bools, you should handle it as if it > was a bool, a ternary operator may be fine. You are right that this is not safe. This is actually unsafe now since prbool can be != 1/0(in this particular case it's ok). C++ bool will upcast to int just fine. Fixed in current patch with explicit 1/0. > > @@ +125,5 @@ > > +#define SQLITE_MULTIPLEX(NAME, DESC) \ > > + SQLITE_TIME_SPENT(OTHER_ ## NAME, DESC) \ > > + SQLITE_TIME_SPENT(PLACES_ ## NAME, DESC) \ > > + SQLITE_TIME_SPENT(COOKIES_ ## NAME, DESC) \ > > + SQLITE_TIME_SPENT(URLCLASSIFIER_ ## NAME, DESC) > > I dunno where the description is used (if it is at all) in telemetry, maybe > you may prefix DESC with "Other: ", "Places: ", "Cookies: ", ... I could do that and it would make sense if the probe descriptions were exposed. At the moment they aren't so it would confuse matters to have string macrology. I think it's clear enough for people who read the source. Once I expose the histogram names to ui, we can revisit this issue. Addressed other comments
Attachment #550156 -
Flags: review?(mak77)
Assignee | ||
Comment 4•13 years ago
|
||
Sorry for the mixup, here is a qrefed patch
Attachment #549922 -
Attachment is obsolete: true
Attachment #550156 -
Attachment is obsolete: true
Attachment #550156 -
Flags: review?(mak77)
Attachment #550419 -
Flags: review?(mak77)
Updated•13 years ago
|
Attachment #550419 -
Flags: review?(mak77) → review+
Assignee | ||
Comment 5•13 years ago
|
||
http://hg.mozilla.org/integration/mozilla-inbound/rev/21dcb3293c2b
Whiteboard: [inbound]
Comment 6•13 years ago
|
||
http://hg.mozilla.org/mozilla-central/rev/21dcb3293c2b
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla8
Comment 7•13 years ago
|
||
Please note that Marco is not yet a peer in storage and you still need to have review from a peer or owner to follow the review policy.
You need to log in
before you can comment on or make changes to this bug.
Description
•