Create WorkerThread, an abstraction to perform some work on an interval.

Create WorkerThread, an abstraction to perform some work on an interval.

This was extracted from CrashReportUploadThread and will be re-used for the
database pruning thread.

BUG=crashpad:22
R=mark@chromium.org

Committed: https://chromium.googlesource.com/crashpad/crashpad/+/6d829e9af7d7630422d8095747905015d347b3be

[Robert Sesek, 2015-12-14 22:52:02]

Patchset #1 (id:1) has been deleted

[Robert Sesek, 2015-12-14 22:52:06]

Patchset #1 (id:20001) has been deleted

[Robert Sesek, 2015-12-14 22:52:10]

Patchset #1 (id:40001) has been deleted

[Robert Sesek, 2015-12-14 22:54:23]

rsesek@chromium.org changed reviewers:
+ mark@chromium.org

[Mark Mentovai, 2015-12-15 17:56:59]

https://codereview.chromium.org/1526563003/diff/60001/handler/crash_report_up...
File handler/crash_report_upload_thread.h (right):

https://codereview.chromium.org/1526563003/diff/60001/handler/crash_report_up...
handler/crash_report_upload_thread.h:98: // WorkerThread::Delegate:
Can you sort this below UploadReport() (and in the .cc too) to make it clear
that only this one thing comes from WorkerThread::Delegate?

https://codereview.chromium.org/1526563003/diff/60001/handler/crash_report_up...
handler/crash_report_upload_thread.h:139: WorkerThread thread_;
Sort before the pointer (before or after url_, as you like).

https://codereview.chromium.org/1526563003/diff/60001/util/thread/worker_thre...
File util/thread/worker_thread.cc (right):

https://codereview.chromium.org/1526563003/diff/60001/util/thread/worker_thre...
util/thread/worker_thread.cc:48: WorkerThread* self_;  // Weak, owns this.
Sort for better packing: Semaphore, double, pointer.

https://codereview.chromium.org/1526563003/diff/60001/util/thread/worker_thre...
util/thread/worker_thread.cc:50: Semaphore semaphore_;
Can we retain the comment we had before?

  // TODO(mark): Use a condition variable instead?

https://codereview.chromium.org/1526563003/diff/60001/util/thread/worker_thre...
util/thread/worker_thread.cc:62: WorkerThread::~WorkerThread() {}
DCHECK(!running_); ?

https://codereview.chromium.org/1526563003/diff/60001/util/thread/worker_thre...
File util/thread/worker_thread.h (right):

https://codereview.chromium.org/1526563003/diff/60001/util/thread/worker_thre...
util/thread/worker_thread.h:25: }
//  namespace internal

https://codereview.chromium.org/1526563003/diff/60001/util/thread/worker_thre...
util/thread/worker_thread.h:44: //! \param[in] work_interval The time interval
in seconds at which the \a
Counting from one call to DoWork() to the next call to DoWork(), or counting
from one return from DoWork() to the next call to DoWork()?

https://codereview.chromium.org/1526563003/diff/60001/util/thread/worker_thre...
util/thread/worker_thread.h:46: //! \param[in] delegate The work delegate to
invoke every the interval.
every the interval

https://codereview.chromium.org/1526563003/diff/60001/util/thread/worker_thre...
util/thread/worker_thread.h:57: void Start(double initial_work_delay);
Is there a reason that the initial delay and the subsequent interval aren’t
specified in the same call? That seems like a friendlier interface.

https://codereview.chromium.org/1526563003/diff/60001/util/thread/worker_thre...
util/thread/worker_thread.h:70: //!     immediately.
Does the next work after “now” happen after a new work_interval, or does it
happen at the originally-scheduled time?

https://codereview.chromium.org/1526563003/diff/60001/util/thread/worker_thre...
File util/thread/worker_thread_test.cc (right):

https://codereview.chromium.org/1526563003/diff/60001/util/thread/worker_thre...
util/thread/worker_thread_test.cc:17: #include <time.h>
Don’t use time(), use ClockMonotonicNanoseconds() from "util/misc/clock.h".

https://codereview.chromium.org/1526563003/diff/60001/util/thread/worker_thre...
util/thread/worker_thread_test.cc:44: WorkerThread thread(0.5, &delegate);
Can you speed this up? If you’re not using 1-second-resolution time(), you
should be able to speed this up substantially.

https://codereview.chromium.org/1526563003/diff/60001/util/thread/worker_thre...
util/thread/worker_thread_test.cc:50: while (delegate.work_count() < 2) {}
Yuck. Can you do something better than a busy-spin loop? A semaphore might
actually be ideal for this. Throughout this file.

https://codereview.chromium.org/1526563003/diff/60001/util/thread/worker_thre...
util/thread/worker_thread_test.cc:54: EXPECT_GE(2, time(nullptr) - now);
Everything named “now” in this file should be “start” or “start_time” or
similar, because things like this line are confusing otherwise.

https://codereview.chromium.org/1526563003/diff/60001/util/thread/worker_thre...
util/thread/worker_thread_test.cc:121: 
Blank line at end of function.

[Robert Sesek, 2015-12-23 19:22:32]

https://codereview.chromium.org/1526563003/diff/60001/handler/crash_report_up...
File handler/crash_report_upload_thread.h (right):

https://codereview.chromium.org/1526563003/diff/60001/handler/crash_report_up...
handler/crash_report_upload_thread.h:98: // WorkerThread::Delegate:
On 2015/12/15 17:56:58, Mark Mentovai wrote:
> Can you sort this below UploadReport() (and in the .cc too) to make it clear
> that only this one thing comes from WorkerThread::Delegate?

Done.

https://codereview.chromium.org/1526563003/diff/60001/handler/crash_report_up...
handler/crash_report_upload_thread.h:139: WorkerThread thread_;
On 2015/12/15 17:56:58, Mark Mentovai wrote:
> Sort before the pointer (before or after url_, as you like).

Done.

https://codereview.chromium.org/1526563003/diff/60001/util/thread/worker_thre...
File util/thread/worker_thread.cc (right):

https://codereview.chromium.org/1526563003/diff/60001/util/thread/worker_thre...
util/thread/worker_thread.cc:48: WorkerThread* self_;  // Weak, owns this.
On 2015/12/15 17:56:58, Mark Mentovai wrote:
> Sort for better packing: Semaphore, double, pointer.

Done.

https://codereview.chromium.org/1526563003/diff/60001/util/thread/worker_thre...
util/thread/worker_thread.cc:50: Semaphore semaphore_;
On 2015/12/15 17:56:58, Mark Mentovai wrote:
> Can we retain the comment we had before?
> 
>   // TODO(mark): Use a condition variable instead?

Done.

https://codereview.chromium.org/1526563003/diff/60001/util/thread/worker_thre...
util/thread/worker_thread.cc:62: WorkerThread::~WorkerThread() {}
On 2015/12/15 17:56:58, Mark Mentovai wrote:
> DCHECK(!running_); ?

Done, but Thread::~Thread also DCHECKs this.

https://codereview.chromium.org/1526563003/diff/60001/util/thread/worker_thre...
File util/thread/worker_thread.h (right):

https://codereview.chromium.org/1526563003/diff/60001/util/thread/worker_thre...
util/thread/worker_thread.h:25: }
On 2015/12/15 17:56:58, Mark Mentovai wrote:
> //  namespace internal

Done.

https://codereview.chromium.org/1526563003/diff/60001/util/thread/worker_thre...
util/thread/worker_thread.h:44: //! \param[in] work_interval The time interval
in seconds at which the \a
On 2015/12/15 17:56:59, Mark Mentovai wrote:
> Counting from one call to DoWork() to the next call to DoWork(), or counting
> from one return from DoWork() to the next call to DoWork()?

Done.

https://codereview.chromium.org/1526563003/diff/60001/util/thread/worker_thre...
util/thread/worker_thread.h:46: //! \param[in] delegate The work delegate to
invoke every the interval.
On 2015/12/15 17:56:59, Mark Mentovai wrote:
> every the interval

Done.

https://codereview.chromium.org/1526563003/diff/60001/util/thread/worker_thre...
util/thread/worker_thread.h:57: void Start(double initial_work_delay);
On 2015/12/15 17:56:59, Mark Mentovai wrote:
> Is there a reason that the initial delay and the subsequent interval aren’t
> specified in the same call? That seems like a friendlier interface.

Yes, I think this interface is friendlier. Currently the interface is "Create
WorkerThread s.t. every N, do X, and Start after M". If both time intervals were
ctor parameters, it's ambiguous as to which is the start interval and which is
the work interval.

https://codereview.chromium.org/1526563003/diff/60001/util/thread/worker_thre...
util/thread/worker_thread.h:70: //!     immediately.
On 2015/12/15 17:56:58, Mark Mentovai wrote:
> Does the next work after “now” happen after a new work_interval, or does it
> happen at the originally-scheduled time?

Done.

https://codereview.chromium.org/1526563003/diff/60001/util/thread/worker_thre...
File util/thread/worker_thread_test.cc (right):

https://codereview.chromium.org/1526563003/diff/60001/util/thread/worker_thre...
util/thread/worker_thread_test.cc:17: #include <time.h>
On 2015/12/15 17:56:59, Mark Mentovai wrote:
> Don’t use time(), use ClockMonotonicNanoseconds() from "util/misc/clock.h".

Done.

https://codereview.chromium.org/1526563003/diff/60001/util/thread/worker_thre...
util/thread/worker_thread_test.cc:44: WorkerThread thread(0.5, &delegate);
On 2015/12/15 17:56:59, Mark Mentovai wrote:
> Can you speed this up? If you’re not using 1-second-resolution time(), you
> should be able to speed this up substantially.

Done.

https://codereview.chromium.org/1526563003/diff/60001/util/thread/worker_thre...
util/thread/worker_thread_test.cc:50: while (delegate.work_count() < 2) {}
On 2015/12/15 17:56:59, Mark Mentovai wrote:
> Yuck. Can you do something better than a busy-spin loop? A semaphore might
> actually be ideal for this. Throughout this file.

Pfft, done.

https://codereview.chromium.org/1526563003/diff/60001/util/thread/worker_thre...
util/thread/worker_thread_test.cc:54: EXPECT_GE(2, time(nullptr) - now);
On 2015/12/15 17:56:59, Mark Mentovai wrote:
> Everything named “now” in this file should be “start” or “start_time” or
> similar, because things like this line are confusing otherwise.

Done.

https://codereview.chromium.org/1526563003/diff/60001/util/thread/worker_thre...
util/thread/worker_thread_test.cc:121: 
On 2015/12/15 17:56:59, Mark Mentovai wrote:
> Blank line at end of function.

Done.

[Mark Mentovai, 2016-01-04 16:01:31]

LGTM

https://codereview.chromium.org/1526563003/diff/60001/util/thread/worker_thre...
File util/thread/worker_thread.cc (right):

https://codereview.chromium.org/1526563003/diff/60001/util/thread/worker_thre...
util/thread/worker_thread.cc:62: WorkerThread::~WorkerThread() {}
Robert Sesek wrote:
> On 2015/12/15 17:56:58, Mark Mentovai wrote:
> > DCHECK(!running_); ?
> 
> Done, but Thread::~Thread also DCHECKs this.

But it’s a different running_.

https://codereview.chromium.org/1526563003/diff/60001/util/thread/worker_thre...
File util/thread/worker_thread.h (right):

https://codereview.chromium.org/1526563003/diff/60001/util/thread/worker_thre...
util/thread/worker_thread.h:57: void Start(double initial_work_delay);
Robert Sesek wrote:
> On 2015/12/15 17:56:59, Mark Mentovai wrote:
> > Is there a reason that the initial delay and the subsequent interval aren’t
> > specified in the same call? That seems like a friendlier interface.
> 
> Yes, I think this interface is friendlier. Currently the interface is "Create
> WorkerThread s.t. every N, do X, and Start after M". If both time intervals
were
> ctor parameters, it's ambiguous as to which is the start interval and which is
> the work interval.

I think it’s still ambiguous this way, it’s just two doubles, and you don’t know
which means what. SetInitialDelay()/SetRepeatDelay() setters?

It’s not horribly important, so if you still have a strong preference for it
as-is, you can keep it.

[Robert Sesek, 2016-01-04 22:09:45]

https://codereview.chromium.org/1526563003/diff/60001/util/thread/worker_thre...
File util/thread/worker_thread.h (right):

https://codereview.chromium.org/1526563003/diff/60001/util/thread/worker_thre...
util/thread/worker_thread.h:57: void Start(double initial_work_delay);
On 2016/01/04 16:01:31, Mark Mentovai wrote:
> Robert Sesek wrote:
> > On 2015/12/15 17:56:59, Mark Mentovai wrote:
> > > Is there a reason that the initial delay and the subsequent interval
aren’t
> > > specified in the same call? That seems like a friendlier interface.
> > 
> > Yes, I think this interface is friendlier. Currently the interface is
"Create
> > WorkerThread s.t. every N, do X, and Start after M". If both time intervals
> were
> > ctor parameters, it's ambiguous as to which is the start interval and which
is
> > the work interval.
> 
> I think it’s still ambiguous this way, it’s just two doubles, and you don’t
know
> which means what. SetInitialDelay()/SetRepeatDelay() setters?
> 
> It’s not horribly important, so if you still have a strong preference for it
> as-is, you can keep it.

I think this as-is is less ambiguous than a two-double ctor, but I think adding
two setters would needlessly complicate things (would require either having
default values or an InitializationStateDcheck). If more than these two params
are required, then maybe an Options struct would be the way forward in the
future. Going to keep this for now.

[Robert Sesek, 2016-01-04 22:11:00]

Description was changed from

==========
Create WorkerThread, an abstraction to perform some work on an interval.

This was extracted from CrashReportUploadThread and will be re-used for the
database pruning thread.

BUG=crashpad:22
==========

to

==========
Create WorkerThread, an abstraction to perform some work on an interval.

This was extracted from CrashReportUploadThread and will be re-used for the
database pruning thread.

BUG=crashpad:22
R=mark@chromium.org

Committed:
https://chromium.googlesource.com/crashpad/crashpad/+/6d829e9af7d7630422d8095...
==========

[Robert Sesek, 2016-01-04 22:11:02]

Committed patchset #2 (id:80001) manually as
6d829e9af7d7630422d8095747905015d347b3be (presubmit successful).