Closed Bug 675737 Opened 8 years ago Closed 8 years ago

Calculate time taken for by write/open/sync per db + main/other thread

Categories

(Toolkit :: Storage, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla8

People

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

References

Details

(Whiteboard: [inbound])

Attachments

(1 file, 2 obsolete files)

Attached patch more precise accounting (obsolete) — 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)
Assignee: nobody → tglek
Status: NEW → ASSIGNED
OS: Linux → All
Hardware: x86 → All
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+
Attached patch name, comment cleanups (obsolete) — Splinter Review
(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)
I think you attached again the previous version of the patch
Attached patch v2 for realSplinter Review
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)
Attachment #550419 - Flags: review?(mak77) → review+
http://hg.mozilla.org/mozilla-central/rev/21dcb3293c2b
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla8
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.