(dormant account) Assignee
	Description • 13 years ago

Tracking io based on file + thread results in a slight combinatorial explosion that's papered over with macros.

	Marco Bonardo [:mak]
	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: ", ...

	(dormant account) 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

	Marco Bonardo [:mak]
	Comment 3 • 13 years ago

I think you attached again the previous version of the patch

	(dormant account) Assignee
	Comment 4 • 13 years ago

Sorry for the mixup, here is a qrefed patch

	(dormant account) Assignee
	Comment 5 • 13 years ago

http://hg.mozilla.org/integration/mozilla-inbound/rev/21dcb3293c2b

	Marco Bonardo [:mak]
	Comment 6 • 13 years ago

http://hg.mozilla.org/mozilla-central/rev/21dcb3293c2b

	Shawn Wilsher :sdwilsh
	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.