Simplify file tracing end events.

Simplify file tracing end events.

R=oysteine@chromium.org
BUG=none
TEST=pathless file trace events with size work

Committed: https://crrev.com/97f619f66b30bcafe0fd211f1e09bcf6ae4f325d
Cr-Commit-Position: refs/heads/master@{#333498}

[oystein (OOO til 10th of July), 2015-06-05 22:06:26]

https://codereview.chromium.org/1149083012/diff/1/content/browser/tracing/fil...
File content/browser/tracing/file_tracing_provider_impl.cc (right):

https://codereview.chromium.org/1149083012/diff/1/content/browser/tracing/fil...
content/browser/tracing/file_tracing_provider_impl.cc:55:
TRACE_VARIED_PARAMS(BEGIN);
The macro is cool, but I'm not really convinced it's worth it :). Is it because
you suspect this'll grow to include even more parameters?

Also, do you even need to pass the same arguments to _END? I assume they don't
change. What happens if you just use TRACE_EVENT_NESTABLE_ASYNC_END0?

[Dan Beam, 2015-06-06 01:57:25]

https://codereview.chromium.org/1149083012/diff/1/content/browser/tracing/fil...
File content/browser/tracing/file_tracing_provider_impl.cc (right):

https://codereview.chromium.org/1149083012/diff/1/content/browser/tracing/fil...
content/browser/tracing/file_tracing_provider_impl.cc:55:
TRACE_VARIED_PARAMS(BEGIN);
On 2015/06/05 22:06:25, Oystein wrote:
> The macro is cool, but I'm not really convinced it's worth it :). Is it
because
> you suspect this'll grow to include even more parameters?
> 
> Also, do you even need to pass the same arguments to _END? I assume they don't
> change. What happens if you just use TRACE_EVENT_NESTABLE_ASYNC_END0? 

I guess I could, but what happens if the params change between beginning and
end?  Should I just remove the params for End()?

I agree that the code doesn't seem to care, it's just looking for ID.

[dsinclair, 2015-06-08 13:05:37]

dsinclair@chromium.org changed reviewers:
+ dsinclair@chromium.org

[dsinclair, 2015-06-08 13:05:38]

https://codereview.chromium.org/1149083012/diff/1/content/browser/tracing/fil...
File content/browser/tracing/file_tracing_provider_impl.cc (right):

https://codereview.chromium.org/1149083012/diff/1/content/browser/tracing/fil...
content/browser/tracing/file_tracing_provider_impl.cc:33: #define
TRACE_VARIED_PARAMS(BEGIN_OR_END) \
I'd prefer not to do this. It makes the code a lot harder to follow. Why not
just always log size, even if it's zero? Or, if it's zero, log as -1 or
something so we know it was unknown.

[Dan Beam, 2015-06-08 18:42:54]

https://codereview.chromium.org/1149083012/diff/1/content/browser/tracing/fil...
File content/browser/tracing/file_tracing_provider_impl.cc (right):

https://codereview.chromium.org/1149083012/diff/1/content/browser/tracing/fil...
content/browser/tracing/file_tracing_provider_impl.cc:33: #define
TRACE_VARIED_PARAMS(BEGIN_OR_END) \
On 2015/06/08 13:05:38, dsinclair wrote:
> I'd prefer not to do this. It makes the code a lot harder to follow. Why not
> just always log size, even if it's zero? Or, if it's zero, log as -1 or
> something so we know it was unknown.

making the code noisier keeps the UI cleaner, but I'm fine to just change this
all to

  TRACE_EVENT_NESABLE_ASYNC_{BEGIN,END}2(..)

if you want there to be a lot of cruft in these events

[oystein (OOO til 10th of July), 2015-06-08 18:53:47]

https://codereview.chromium.org/1149083012/diff/1/content/browser/tracing/fil...
File content/browser/tracing/file_tracing_provider_impl.cc (right):

https://codereview.chromium.org/1149083012/diff/1/content/browser/tracing/fil...
content/browser/tracing/file_tracing_provider_impl.cc:55:
TRACE_VARIED_PARAMS(BEGIN);
On 2015/06/06 01:57:25, Dan Beam wrote:
> On 2015/06/05 22:06:25, Oystein wrote:
> > The macro is cool, but I'm not really convinced it's worth it :). Is it
> because
> > you suspect this'll grow to include even more parameters?
> > 
> > Also, do you even need to pass the same arguments to _END? I assume they
don't
> > change. What happens if you just use TRACE_EVENT_NESTABLE_ASYNC_END0? 
> 
> I guess I could, but what happens if the params change between beginning and
> end?  Should I just remove the params for End()?
> 
> I agree that the code doesn't seem to care, it's just looking for ID.

Hmm... In what situations would the params change? From what I can tell (I may
be missing something) these things are only set on initialization. If so I would
just include the params in the _BEGIN(X) and use _END0 without them (removing
the need for the macro too).

[dsinclair, 2015-06-08 19:17:37]

On 2015/06/08 at 18:42:54, dbeam wrote:
>
https://codereview.chromium.org/1149083012/diff/1/content/browser/tracing/fil...
> File content/browser/tracing/file_tracing_provider_impl.cc (right):
> 
>
https://codereview.chromium.org/1149083012/diff/1/content/browser/tracing/fil...
> content/browser/tracing/file_tracing_provider_impl.cc:33: #define
TRACE_VARIED_PARAMS(BEGIN_OR_END) \
> On 2015/06/08 13:05:38, dsinclair wrote:
> > I'd prefer not to do this. It makes the code a lot harder to follow. Why not
> > just always log size, even if it's zero? Or, if it's zero, log as -1 or
> > something so we know it was unknown.
> 
> making the code noisier keeps the UI cleaner, but I'm fine to just change this
all to
> 
>   TRACE_EVENT_NESABLE_ASYNC_{BEGIN,END}2(..)
> 
> if you want there to be a lot of cruft in these events


Not sure what you mean by a lot of cruft, it's a single size variable? There
won't ever be more as we have a max of 2 arguments to trace events unless you
use a ConvertableToTraceFormat.

[Dan Beam, 2015-06-08 19:18:46]

On 2015/06/08 19:17:37, dsinclair wrote:
> On 2015/06/08 at 18:42:54, dbeam wrote:
> >
>
https://codereview.chromium.org/1149083012/diff/1/content/browser/tracing/fil...
> > File content/browser/tracing/file_tracing_provider_impl.cc (right):
> > 
> >
>
https://codereview.chromium.org/1149083012/diff/1/content/browser/tracing/fil...
> > content/browser/tracing/file_tracing_provider_impl.cc:33: #define
> TRACE_VARIED_PARAMS(BEGIN_OR_END) \
> > On 2015/06/08 13:05:38, dsinclair wrote:
> > > I'd prefer not to do this. It makes the code a lot harder to follow. Why
not
> > > just always log size, even if it's zero? Or, if it's zero, log as -1 or
> > > something so we know it was unknown.
> > 
> > making the code noisier keeps the UI cleaner, but I'm fine to just change
this
> all to
> > 
> >   TRACE_EVENT_NESABLE_ASYNC_{BEGIN,END}2(..)
> > 
> > if you want there to be a lot of cruft in these events
> 
> 
> Not sure what you mean by a lot of cruft, it's a single size variable? There
> won't ever be more as we have a max of 2 arguments to trace events unless you
> use a ConvertableToTraceFormat.

There will be some:

Path: ""
Size: 0

when looking at events (which isn't very helpful).

[Dan Beam, 2015-06-08 19:22:28]

https://codereview.chromium.org/1149083012/diff/1/content/browser/tracing/fil...
File content/browser/tracing/file_tracing_provider_impl.cc (right):

https://codereview.chromium.org/1149083012/diff/1/content/browser/tracing/fil...
content/browser/tracing/file_tracing_provider_impl.cc:55:
TRACE_VARIED_PARAMS(BEGIN);
On 2015/06/08 18:53:47, Oystein wrote:
> On 2015/06/06 01:57:25, Dan Beam wrote:
> > On 2015/06/05 22:06:25, Oystein wrote:
> > > The macro is cool, but I'm not really convinced it's worth it :). Is it
> > because
> > > you suspect this'll grow to include even more parameters?
> > > 
> > > Also, do you even need to pass the same arguments to _END? I assume they
> don't
> > > change. What happens if you just use TRACE_EVENT_NESTABLE_ASYNC_END0? 
> > 
> > I guess I could, but what happens if the params change between beginning and
> > end?  Should I just remove the params for End()?
> > 
> > I agree that the code doesn't seem to care, it's just looking for ID.
> 
> Hmm... In what situations would the params change? From what I can tell (I may
> be missing something) these things are only set on initialization. If so I
would
> just include the params in the _BEGIN(X) and use _END0 without them (removing
> the need for the macro too).

The current implementation wouldn't, but this level of the code doesn't
know/can't guarantee that.  It'd also be nice to eventually log the actual
amount of bytes read instead of the maximum allowed (e.g you ask for 4096 bytes
but there's only 1 left).

[dsinclair, 2015-06-08 19:22:40]

On 2015/06/08 at 19:18:46, dbeam wrote:
> On 2015/06/08 19:17:37, dsinclair wrote:
> > On 2015/06/08 at 18:42:54, dbeam wrote:
> > >
> >
https://codereview.chromium.org/1149083012/diff/1/content/browser/tracing/fil...
> > > File content/browser/tracing/file_tracing_provider_impl.cc (right):
> > > 
> > >
> >
https://codereview.chromium.org/1149083012/diff/1/content/browser/tracing/fil...
> > > content/browser/tracing/file_tracing_provider_impl.cc:33: #define
> > TRACE_VARIED_PARAMS(BEGIN_OR_END) \
> > > On 2015/06/08 13:05:38, dsinclair wrote:
> > > > I'd prefer not to do this. It makes the code a lot harder to follow. Why
not
> > > > just always log size, even if it's zero? Or, if it's zero, log as -1 or
> > > > something so we know it was unknown.
> > > 
> > > making the code noisier keeps the UI cleaner, but I'm fine to just change
this
> > all to
> > > 
> > >   TRACE_EVENT_NESABLE_ASYNC_{BEGIN,END}2(..)
> > > 
> > > if you want there to be a lot of cruft in these events
> > 
> > 
> > Not sure what you mean by a lot of cruft, it's a single size variable? There
> > won't ever be more as we have a max of 2 arguments to trace events unless
you
> > use a ConvertableToTraceFormat.
> 
> There will be some:
> 
> Path: ""
> Size: 0
> 
> when looking at events (which isn't very helpful).


I think that's fine. It seems more useful to see that the data was recorded and
it's blank then to wonder if you missed a category and it didn't record.

[Dan Beam, 2015-06-08 19:26:53]

On 2015/06/08 19:22:40, dsinclair wrote:
> On 2015/06/08 at 19:18:46, dbeam wrote:
> > On 2015/06/08 19:17:37, dsinclair wrote:
> > > On 2015/06/08 at 18:42:54, dbeam wrote:
> > > >
> > >
>
https://codereview.chromium.org/1149083012/diff/1/content/browser/tracing/fil...
> > > > File content/browser/tracing/file_tracing_provider_impl.cc (right):
> > > > 
> > > >
> > >
>
https://codereview.chromium.org/1149083012/diff/1/content/browser/tracing/fil...
> > > > content/browser/tracing/file_tracing_provider_impl.cc:33: #define
> > > TRACE_VARIED_PARAMS(BEGIN_OR_END) \
> > > > On 2015/06/08 13:05:38, dsinclair wrote:
> > > > > I'd prefer not to do this. It makes the code a lot harder to follow.
Why
> not
> > > > > just always log size, even if it's zero? Or, if it's zero, log as -1
or
> > > > > something so we know it was unknown.
> > > > 
> > > > making the code noisier keeps the UI cleaner, but I'm fine to just
change
> this
> > > all to
> > > > 
> > > >   TRACE_EVENT_NESABLE_ASYNC_{BEGIN,END}2(..)
> > > > 
> > > > if you want there to be a lot of cruft in these events
> > > 
> > > 
> > > Not sure what you mean by a lot of cruft, it's a single size variable?
There
> > > won't ever be more as we have a max of 2 arguments to trace events unless
> you
> > > use a ConvertableToTraceFormat.
> > 
> > There will be some:
> > 
> > Path: ""
> > Size: 0
> > 
> > when looking at events (which isn't very helpful).
> 
> 
> I think that's fine. It seems more useful to see that the data was recorded
and
> it's blank then to wonder if you missed a category and it didn't record.

Yeah, I thought about that after I posted: I guess it does convey slightly more
information.  I guess I'm just worried about folks thinking it's a bug that the
path is empty or that size is 0.

Anyways, it seems like everybody just wants the simpler implementation for now
anyways, so probably just:

  TRACE_EVENT_NESTABLE_ASYNC_BEGIN2() + TRACE_EVENT_NESTABLE_ASYNC_END0()

is probably the simplest

[Dan Beam, 2015-06-08 21:10:30]

https://codereview.chromium.org/1149083012/diff/20001/base/files/file_tracing.cc
File base/files/file_tracing.cc (right):

https://codereview.chromium.org/1149083012/diff/20001/base/files/file_tracing...
base/files/file_tracing.cc:43: if (!g_provider)
this prevents ends without corresponding begins

https://codereview.chromium.org/1149083012/diff/20001/base/files/file_tracing.h
File base/files/file_tracing.h (left):

https://codereview.chromium.org/1149083012/diff/20001/base/files/file_tracing...
base/files/file_tracing.h:76: bool initialized_;
we decided to axe this for simpler code

https://codereview.chromium.org/1149083012/diff/20001/base/trace_event/trace_...
File base/trace_event/trace_event.h (right):

https://codereview.chromium.org/1149083012/diff/20001/base/trace_event/trace_...
base/trace_event/trace_event.h:740: #define
TRACE_EVENT_NESTABLE_ASYNC_END2(category_group, name, id, arg1_name, \
p.s. there's only 1 other use of this
https://code.google.com/p/chromium/codesearch#chromium/src/net/log/trace_net_...

/if/ the params sent at the end don't matter so much, we should probably try to
remove this to discourage passing around useless params and make it clearer they
aren't necessary.

[oystein (OOO til 10th of July), 2015-06-09 00:05:25]

lgtm

[Dan Beam, 2015-06-09 00:23:44]

dbeam@chromium.org changed reviewers:
- dsinclair@chromium.org

[Dan Beam, 2015-06-09 00:25:13]

dbeam@chromium.org changed reviewers:
+ dsinclair@chromium.org, thestig@chromium.org

[Dan Beam, 2015-06-09 00:25:14]

+thestig for base/files
+dsinclair for base/trace_event, content/browser/tracing

[Lei Zhang, 2015-06-09 00:44:02]

https://codereview.chromium.org/1149083012/diff/20001/base/files/file_tracing.h
File base/files/file_tracing.h (right):

https://codereview.chromium.org/1149083012/diff/20001/base/files/file_tracing...
base/files/file_tracing.h:74: // The ID of this trace.
Does this have any lifetime requirements like |file_| ?

https://codereview.chromium.org/1149083012/diff/20001/base/files/file_tracing...
base/files/file_tracing.h:77: // The name of the event to trace (e.g. "Read",
"Write").
No longer needs the "File" prefix?

[Dan Beam, 2015-06-09 00:49:09]

https://codereview.chromium.org/1149083012/diff/20001/base/files/file_tracing.h
File base/files/file_tracing.h (right):

https://codereview.chromium.org/1149083012/diff/20001/base/files/file_tracing...
base/files/file_tracing.h:74: // The ID of this trace.
On 2015/06/09 00:44:02, Lei Zhang wrote:
> Does this have any lifetime requirements like |file_| ?

Yeah, added more doc.

https://codereview.chromium.org/1149083012/diff/20001/base/files/file_tracing...
base/files/file_tracing.h:77: // The name of the event to trace (e.g. "Read",
"Write").
On 2015/06/09 00:44:02, Lei Zhang wrote:
> No longer needs the "File" prefix?

whoops, comment added back (still prefixed)

[Lei Zhang, 2015-06-09 00:51:25]

lgtm

[dsinclair, 2015-06-09 13:43:33]

The CQ bit was checked by dsinclair@chromium.org

[dsinclair, 2015-06-09 13:43:33]

lgtm

[dsinclair, 2015-06-09 13:43:34]

The patchset sent to the CQ was uploaded after l-g-t-m from
oysteine@chromium.org
Link to the patchset: https://codereview.chromium.org/1149083012/#ps40001
(title: "thestig@ review")

[commit-bot: I haz the power, 2015-06-09 13:43:49]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1149083012/40001

[commit-bot: I haz the power, 2015-06-09 16:08:23]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2015-06-09 16:09:05]

Patchset 3 (id:??) landed as
https://crrev.com/97f619f66b30bcafe0fd211f1e09bcf6ae4f325d
Cr-Commit-Position: refs/heads/master@{#333498}