Use FILE thread for disk operations in TraceSubscriberStdio

Use SequencedWorkerPool for disk operations in TraceSubscriberStdio.

This class was hitting a ThreadRestrictions assert because it called
OpenFile on the UI thread. To reduce unnecessary copying, I've changed
the OnTraceDataCollected argument from std::string to RefCountedString.

BUG=None
TEST=content_unittests

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=123140

[jam, 2012-02-06 20:25:28]

http://codereview.chromium.org/9333003/diff/1/content/browser/trace_subscribe...
File content/browser/trace_subscriber_stdio.cc (right):

http://codereview.chromium.org/9333003/diff/1/content/browser/trace_subscribe...
content/browser/trace_subscriber_stdio.cc:27:
base::ThreadRestrictions::ScopedAllowIO scoped_allow_io;
the goal is to eventually remove ThreadRestrictions. adding more instances moves
that work to someone else.

[joth, 2012-02-07 09:14:59]

http://codereview.chromium.org/9333003/diff/1/content/browser/trace_subscribe...
File content/browser/trace_subscriber_stdio.cc (right):

http://codereview.chromium.org/9333003/diff/1/content/browser/trace_subscribe...
content/browser/trace_subscriber_stdio.cc:27:
base::ThreadRestrictions::ScopedAllowIO scoped_allow_io;
On 2012/02/06 20:25:29, John Abd-El-Malek wrote:
> the goal is to eventually remove ThreadRestrictions. adding more instances
moves
> that work to someone else.

I understand what you're saying here, but several established uses of it are
commented with words to the effect "this is by design" so I don't think in
reality it's ever going to be removed? e.g.:

http://code.google.com/codesearch#OAMlx_jo-ck/src/base/process_util_linux.cc&...
http://code.google.com/codesearch#OAMlx_jo-ck/src/base/shared_memory_posix.cc...
http://code.google.com/codesearch#OAMlx_jo-ck/src/chrome/browser/rlz/rlz.cc&e...

[lain Merrick, 2012-02-07 11:14:06]

TraceSubscriberStdio could potentially use a background thread, but that would
involve copying the std::string argument of OnTraceDataCollected. It turns out
that TraceController batches up the trace data and sends it all in a single
massive fragment, so we might have to copy many megs of data.

I'll try changing that std::string to some other type.

[Iain Merrick, 2012-02-08 12:07:05]

This is now a much bigger change. I've noted a couple of places where I'm not
sure I've used the best approach.

Unit test passes. I will also check it on the Android port, which uses this
class for its TraceEvent implementation.

Joth, is there test coverage for startup tracing? If not, what manual tests
should I do?

https://chromiumcodereview.appspot.com/9333003/diff/5001/base/debug/trace_eve...
File base/debug/trace_event_impl.h (right):

https://chromiumcodereview.appspot.com/9333003/diff/5001/base/debug/trace_eve...
base/debug/trace_event_impl.h:203: typedef base::Callback<void(const
scoped_refptr<base::RefCountedString>&)>
Previously we were converting from RefCountedData -> string, now we just pass
RefCountedString with no conversion.

The only difference is that RefCountedData is a subclass of RefCounted, whereas
RefCountedString is RefCountedThreadSafe. Joth, do you think that will cause any
issues for the startup tracing?

https://chromiumcodereview.appspot.com/9333003/diff/5001/content/browser/trac...
File content/browser/trace_controller.h (right):

https://chromiumcodereview.appspot.com/9333003/diff/5001/content/browser/trac...
content/browser/trace_controller.h:159: const
scoped_refptr<base::RefCountedString>& events_str_ptr);
Left it like this for consistency, but it seems like we could just pass "const
RefCountedString&". Worth changing?

https://chromiumcodereview.appspot.com/9333003/diff/5001/content/browser/trac...
File content/browser/trace_message_filter.cc (right):

https://chromiumcodereview.appspot.com/9333003/diff/5001/content/browser/trac...
content/browser/trace_message_filter.cc:100: string->data() = data;
This is a copy, not sure if there's a way to avoid it. Can we take ownership of
strings from IPC messages?

[joth, 2012-02-08 13:22:20]

> Joth, is there test coverage for startup tracing? If not, what manual tests
> should I do?

chrome --trace-startup should be sufficient. (this will drop a chrometrace.log
which you should be able to open in chrome://tracing)

https://chromiumcodereview.appspot.com/9333003/diff/5001/base/debug/trace_eve...
File base/debug/trace_event_impl.h (right):

https://chromiumcodereview.appspot.com/9333003/diff/5001/base/debug/trace_eve...
base/debug/trace_event_impl.h:203: typedef base::Callback<void(const
scoped_refptr<base::RefCountedString>&)>
On 2012/02/08 12:07:05, Iain Merrick wrote:
> Previously we were converting from RefCountedData -> string, now we just pass
> RefCountedString with no conversion.
> 
> The only difference is that RefCountedData is a subclass of RefCounted,
whereas
> RefCountedString is RefCountedThreadSafe. Joth, do you think that will cause
any
> issues for the startup tracing?

Should be fine. This sounds a good change.

https://chromiumcodereview.appspot.com/9333003/diff/5001/content/browser/trac...
File content/browser/trace_controller.h (right):

https://chromiumcodereview.appspot.com/9333003/diff/5001/content/browser/trac...
content/browser/trace_controller.h:159: const
scoped_refptr<base::RefCountedString>& events_str_ptr);
On 2012/02/08 12:07:05, Iain Merrick wrote:
> Left it like this for consistency, but it seems like we could just pass "const
> RefCountedString&". Worth changing?

If you want to allow the implementation of this method to take a ref-count,
you'd need to pass RefCountedString* (non-const) instead.
There's on going debate about the preferred style,
(http://groups.google.com/a/chromium.org/group/chromium-dev/browse_thread/thre...

https://groups.google.com/a/chromium.org/group/chromium-dev/browse_thread/thr...)
so while that's open I agree best just to leave existing code as is to avoid
churning it back & forth.

https://chromiumcodereview.appspot.com/9333003/diff/5001/content/browser/trac...
File content/browser/trace_message_filter.cc (right):

https://chromiumcodereview.appspot.com/9333003/diff/5001/content/browser/trac...
content/browser/trace_message_filter.cc:99:
scoped_refptr<base::RefCountedString> string(new base::RefCountedString());
avoid calling is 'string' as that's already overloaded.

https://chromiumcodereview.appspot.com/9333003/diff/5001/content/browser/trac...
content/browser/trace_message_filter.cc:100: string->data() = data;
On 2012/02/08 12:07:05, Iain Merrick wrote:
> This is a copy, not sure if there's a way to avoid it. Can we take ownership
of
> strings from IPC messages?

At least you're not adding a new copy ..
I think it could work if you make data a non-const ref. (opps!) because then you
could use RefCountedString::TakeString()

https://chromiumcodereview.appspot.com/9333003/diff/5001/content/browser/trac...
File content/browser/trace_message_filter.h (right):

https://chromiumcodereview.appspot.com/9333003/diff/5001/content/browser/trac...
content/browser/trace_message_filter.h:11: #include
"base/memory/ref_counted_memory.h"
needed?

https://chromiumcodereview.appspot.com/9333003/diff/5001/content/browser/trac...
File content/browser/trace_subscriber_stdio.cc (right):

https://chromiumcodereview.appspot.com/9333003/diff/5001/content/browser/trac...
content/browser/trace_subscriber_stdio.cc:13: // All method calls on this class
should be done on the FILE thread.
FWIW the new hotness is to use SequencedWorkerPool but given this is 'debug
only' the old school way seems fine to me...

https://chromiumcodereview.appspot.com/9333003/diff/5001/content/browser/trac...
content/browser/trace_subscriber_stdio.cc:82: // This can't be inlined because
TraceSubscriberStdioImpl isn't public.
no need to comment this: it is convention to *always* put d'tor in the cc file
for this amongst other reasons.

https://chromiumcodereview.appspot.com/9333003/diff/5001/content/browser/trac...
content/browser/trace_subscriber_stdio.cc:86:
BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE, base::Bind(
I think we'd normally break line before bind and then line up its params too.
(and again below)

[joth, 2012-02-08 13:26:01]

On 2012/02/08 13:22:20, joth wrote:
> On 2012/02/08 12:07:05, Iain Merrick wrote:
> > This is a copy, not sure if there's a way to avoid it. Can we take ownership
> of
> > strings from IPC messages?
> 
> At least you're not adding a new copy ..
> I think it could work if you make data a non-const ref. (opps!) because then
you
> could use RefCountedString::TakeString()
> 
> 

nah... looks like the IPC macros force one extra string copy on the receiver
side, unfortunately.

[Iain Merrick, 2012-02-08 14:11:24]

https://chromiumcodereview.appspot.com/9333003/diff/5001/content/browser/trac...
File content/browser/trace_message_filter.cc (right):

https://chromiumcodereview.appspot.com/9333003/diff/5001/content/browser/trac...
content/browser/trace_message_filter.cc:99:
scoped_refptr<base::RefCountedString> string(new base::RefCountedString());
On 2012/02/08 13:22:20, joth wrote:
> avoid calling is 'string' as that's already overloaded.

Done.

https://chromiumcodereview.appspot.com/9333003/diff/5001/content/browser/trac...
content/browser/trace_message_filter.cc:100: string->data() = data;
On 2012/02/08 13:22:20, joth wrote:
> On 2012/02/08 12:07:05, Iain Merrick wrote:
> > This is a copy, not sure if there's a way to avoid it. Can we take ownership
> of
> > strings from IPC messages?
> 
> At least you're not adding a new copy ..
> I think it could work if you make data a non-const ref. (opps!) because then
you
> could use RefCountedString::TakeString()

But I only have a const IPC::Message, so yeah, sadly not possible. Makes sense
as this is a MessageFilter, and the same Message will be passed to other
filters.

https://chromiumcodereview.appspot.com/9333003/diff/5001/content/browser/trac...
File content/browser/trace_message_filter.h (right):

https://chromiumcodereview.appspot.com/9333003/diff/5001/content/browser/trac...
content/browser/trace_message_filter.h:11: #include
"base/memory/ref_counted_memory.h"
On 2012/02/08 13:22:20, joth wrote:
> needed?

Oops, no. Removed.

https://chromiumcodereview.appspot.com/9333003/diff/5001/content/browser/trac...
File content/browser/trace_subscriber_stdio.cc (right):

https://chromiumcodereview.appspot.com/9333003/diff/5001/content/browser/trac...
content/browser/trace_subscriber_stdio.cc:13: // All method calls on this class
should be done on the FILE thread.
On 2012/02/08 13:22:20, joth wrote:
> FWIW the new hotness is to use SequencedWorkerPool

Huh, really? The new hotness needs better advertising! Maybe it should give
AdWords a try.

> but given this is 'debug
> only' the old school way seems fine to me...

OK, leaving as is.

https://chromiumcodereview.appspot.com/9333003/diff/5001/content/browser/trac...
content/browser/trace_subscriber_stdio.cc:82: // This can't be inlined because
TraceSubscriberStdioImpl isn't public.
On 2012/02/08 13:22:20, joth wrote:
> no need to comment this: it is convention to *always* put d'tor in the cc file
> for this amongst other reasons.

Done.

https://chromiumcodereview.appspot.com/9333003/diff/5001/content/browser/trac...
content/browser/trace_subscriber_stdio.cc:86:
BrowserThread::PostTask(BrowserThread::FILE, FROM_HERE, base::Bind(
On 2012/02/08 13:22:20, joth wrote:
> I think we'd normally break line before bind and then line up its params too.
> (and again below)

Done.

[joth, 2012-02-08 14:40:18]

lg from my side, back to jam for his thought.
copying in jbates & nduca FYI, and eventually you'll also need to add a base
OWNER too :)

> > FWIW the new hotness is to use SequencedWorkerPool
> 
> Huh, really? The new hotness needs better advertising! Maybe it should give
> AdWords a try.

here you are: 
http://groups.google.com/a/chromium.org/group/chromium-dev/browse_thread/thre...

Actually that thread does indicate that FILE is dead and new code shouldn't use
it :-|
(I think to support that we should really have a comment added in
browser_thread.h to this effect, and I'd love to see that TaskRunner interface
introduced first too.)

[Iain Merrick, 2012-02-08 14:50:05]

On 2012/02/08 14:40:18, joth wrote:
[...]
> Actually that thread does indicate that FILE is dead and new code shouldn't
use
> it :-|
> (I think to support that we should really have a comment added in
> browser_thread.h to this effect, and I'd love to see that TaskRunner interface
> introduced first too.)

The explanation makes a lot of sense. Sounds easy enough, I'll give it a try.

[Iain Merrick, 2012-02-08 15:50:12]

FYI, using scoped_refptr<RefCountedString> in the IPC message itself looks
feasible. Just a little extra template specialization needed. I'll do this in a
separate patch.

[jbates, 2012-02-08 18:05:16]

On 2012/02/08 15:50:12, Iain Merrick wrote:
> FYI, using scoped_refptr<RefCountedString> in the IPC message itself looks
> feasible. Just a little extra template specialization needed. I'll do this in
a
> separate patch.

Your first CL looked good to me. It's slightly sad to see the complexity in the
latest considering that trace-to-file is only used by devs, but I'm not sure
what the best solution is if ThreadRestrictions is banned :p. Btw, the latest CL
is actually increasing the number of string copies (by about 1 per child
process) for the common tracing use cases, so I'm looking forward to your
RefCountedString IPC change.

[Iain Merrick, 2012-02-08 18:15:27]

On 2012/02/08 18:05:16, jbates wrote:
> On 2012/02/08 15:50:12, Iain Merrick wrote:
> > FYI, using scoped_refptr<RefCountedString> in the IPC message itself looks
> > feasible. Just a little extra template specialization needed. I'll do this
in
> a
> > separate patch.
> 
> Your first CL looked good to me. It's slightly sad to see the complexity in
the
> latest considering that trace-to-file is only used by devs, but I'm not sure
> what the best solution is if ThreadRestrictions is banned :p. Btw, the latest
CL
> is actually increasing the number of string copies (by about 1 per child
> process) for the common tracing use cases, so I'm looking forward to your
> RefCountedString IPC change.

I don't think the complexity came out too bad in the end -- SequencedWorkerPool
is nice. We've been using TraceEvent heavily to profile the Android port (which
I can finally discuss in the open, yay) so I definitely want to keep this
feature well-maintained.

[joth, 2012-02-08 18:37:36]

https://chromiumcodereview.appspot.com/9333003/diff/10007/content/browser/tra...
File content/browser/trace_subscriber_stdio.cc (right):

https://chromiumcodereview.appspot.com/9333003/diff/10007/content/browser/tra...
content/browser/trace_subscriber_stdio.cc:78: "TraceSubscriberStdio", FROM_HERE,
Let's extract a constant for the token, else it would be very easy for one to
get changed.
I'd be slightly tempted to use __FILE__ as the value as that's likely to be
sufficiently unique, and also likely already in the binary anyway for logging
etc. purposes.

[Iain Merrick, 2012-02-08 18:57:06]

https://chromiumcodereview.appspot.com/9333003/diff/10007/content/browser/tra...
File content/browser/trace_subscriber_stdio.cc (right):

https://chromiumcodereview.appspot.com/9333003/diff/10007/content/browser/tra...
content/browser/trace_subscriber_stdio.cc:78: "TraceSubscriberStdio", FROM_HERE,
On 2012/02/08 18:37:36, joth wrote:
> Let's extract a constant for the token, else it would be very easy for one to
> get changed.
> I'd be slightly tempted to use __FILE__ as the value as that's likely to be
> sufficiently unique, and also likely already in the binary anyway for logging
> etc. purposes.

__FILE__ is a great idea, done.

[joth, 2012-02-13 21:55:50]

On 8 February 2012 14:40, <joth@chromium.org> wrote:

> lg from my side, back to jam for his thought.
> copying in jbates & nduca FYI, and eventually you'll also need to add a
> base
> OWNER too :)
>
>
>  > FWIW the new hotness is to use SequencedWorkerPool
>>
>
>  Huh, really? The new hotness needs better advertising! Maybe it should
>> give
>> AdWords a try.
>>
>
> here you are:
> http://groups.google.com/a/**chromium.org/group/chromium-**
>
dev/browse_thread/thread/**39a03ef6b9e33aa2/**cd54f52047695edc<http://groups.google.com/a/chromium.org/group/chromium-dev/browse_thread/thread/39a03ef6b9e33aa2/cd54f52047695edc>
>
> Actually that thread does indicate that FILE is dead and new code
> shouldn't use
> it :-|
> (I think to support that we should really have a comment added in
> browser_thread.h to this effect, and I'd love to see that TaskRunner
> interface
> introduced first too.)
>
>
> awesome... it's on its way
https://groups.google.com/a/chromium.org/group/chromium-dev/msg/61cbaa91a0182a3b
:-)



>
>
https://chromiumcodereview.**appspot.com/9333003/<https://chromiumcodereview....
>

[Iain Merrick, 2012-02-17 00:13:39]

John, do you have time to take another look at this? I think I've addressed all
the outstanding comments.

I see I'll have to get separate OWNERS reviews for base/debug and chrome/test,
but I'll wait until you're happy.

[jam, 2012-02-17 23:56:33]

lgtm

http://codereview.chromium.org/9333003/diff/1/content/browser/trace_subscribe...
File content/browser/trace_subscriber_stdio.cc (right):

http://codereview.chromium.org/9333003/diff/1/content/browser/trace_subscribe...
content/browser/trace_subscriber_stdio.cc:27:
base::ThreadRestrictions::ScopedAllowIO scoped_allow_io;
On 2012/02/07 09:14:59, joth wrote:
> On 2012/02/06 20:25:29, John Abd-El-Malek wrote:
> > the goal is to eventually remove ThreadRestrictions. adding more instances
> moves
> > that work to someone else.
> 
> I understand what you're saying here, but several established uses of it are
> commented with words to the effect "this is by design" so I don't think in
> reality it's ever going to be removed? e.g.:

If we keep it because a few places are valid, then it'll get abused much more.

I prefer that you don't add this, and instead do this on another thread.
Eventually we want to remove all these, and this just adds work. But I won't let
it block this commit.

> 
>
http://code.google.com/codesearch#OAMlx_jo-ck/src/base/process_util_linux.cc&...
>
http://code.google.com/codesearch#OAMlx_jo-ck/src/base/shared_memory_posix.cc...
>
http://code.google.com/codesearch#OAMlx_jo-ck/src/chrome/browser/rlz/rlz.cc&e...

[Iain Merrick, 2012-02-17 23:59:32]

http://codereview.chromium.org/9333003/diff/1/content/browser/trace_subscribe...
File content/browser/trace_subscriber_stdio.cc (right):

http://codereview.chromium.org/9333003/diff/1/content/browser/trace_subscribe...
content/browser/trace_subscriber_stdio.cc:27:
base::ThreadRestrictions::ScopedAllowIO scoped_allow_io;
On 2012/02/17 23:56:34, John Abd-El-Malek wrote:
> I prefer that you don't add this, and instead do this on another thread.
> Eventually we want to remove all these, and this just adds work. But I won't
let
> it block this commit.

Removed in latest patch.

[Iain Merrick, 2012-02-18 00:14:37]

+willchan for base/OWNERS
+jcivelli for chrome/test/OWNERS

Hi guys, do you have time to review this patch? Thanks!

[willchan no longer on Chromium, 2012-02-18 18:10:38]

base/ changes LGTM

[Jay Civelli, 2012-02-22 18:58:42]

LGTM for test changes

[commit-bot: I haz the power, 2012-02-22 19:00:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/husky@chromium.org/9333003/12011

[commit-bot: I haz the power, 2012-02-22 19:01:05]

Presubmit check for 9333003-12011 failed and returned exit status 1.

Running presubmit commit checks ...

** Presubmit ERRORS **
Missing LGTM from an OWNER for: chrome/browser/ui/webui/tracing_ui.cc

Presubmit checks took 1.8s to calculate.

[James Hawkins, 2012-02-22 19:08:04]

lgtm

[commit-bot: I haz the power, 2012-02-22 19:08:55]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/husky@chromium.org/9333003/12011

[commit-bot: I haz the power, 2012-02-22 19:20:40]

Try job failure for 9333003-12011 (retry) on android for steps "build, Compile".
It's a second try, previously, steps "build, Compile" failed.
http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=android&nu...

[Iain Merrick, 2012-02-22 22:05:04]

Bitrot repaired, let's try again.

[commit-bot: I haz the power, 2012-02-22 22:05:20]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/husky@chromium.org/9333003/22002

[commit-bot: I haz the power, 2012-02-22 23:53:03]

Change committed as 123140