Issue #4711 - Rewrite legacy thread facilities

Issue #4711 - Rewrite legacy thread facilities

Remove classes `Thread`, `Mutex`, and `Lock` and all tests for them. Remove
all associated files. Replace `AdblockPlus::Sleep()` with
`std::this_thread::sleep_for()`.

Separate concerns by unlinking task specification, memory allocation, and
scheduling. Add class `Scheduler`, which runs tasks with the same policy as
before, namely, run immediately in a single-use thread. This scheduler is
intended to be tranistory.

Replace derivation from class `Thread` with writing tasks as function objects.
In practice all this means is renaming old `Run` functions to `operator()`.
Rename all names "...Thread" to "...Task".

Improve memory handling for tasks by using `shared_ptr` instead of raw `new`
and `delete`. Introduce adapter class `HeapFunction` to replicate the existing
pattern of memory allocation. Add utility function `MakeHeapFunction`.

[Eric, 2016-12-07 18:06:25]

This change cleans up code structure while eliminating the old `Thread` code.
Overall, these changes are rather straightforward. I didn't put in anything I
thought might be controversial.

The old code intermingle three distinct concerns.

(C1) Memory was being allocated by the user and deallocated in the object.
That's a job for smart pointers, which is what the new code uses. It uses
`shared_ptr` in a straightforward way that was simple to write. It's perhaps not
the most efficient, but it may not be permanent code either, so I avoided
premature optimization here.

(C2) Specifying the task was done with a base class and virtual function. While
this is acceptable, there's more flexibility in using function objects, so
that's what this code uses.

(C3) Scheduling execution was internal to the task object rather than separated
out, so there was no good way to manage scheduling. In particular, there was no
central way to join all the threads to ensure completion.

There are some future directions that came into focus while writing this code.

(F1) Task specification should be tied to engine (or isolate or context ...) in
some way. This would allow tracking how many tasks are associated with each. The
most direct way to do this is give each engine a factory to produce tasks,
ensuring reliable tracking of dependency. The engine could also take
responsibility for task object lifetime.

(F2) The scheduler in the present code is global, which is expeditious but not
optimal. The scheduler should likely also be accessible only through the engine.

(F3) Before we get into anything like fancy scheduling, we can keep a container
of pending tasks and join them a destructor.

We have, in addition, different kinds of tasks with their own acceptable
behaviors for task termination at shutdown. This assume that we're not rewriting
JS code (a rather larger task

(T1) We can wait for a task to complete. The file system tasks write, move, and
delete are in the category. (Note: We don't have atomic operations for these;
see #1582. If we did, we might allow these to be interruptible.) The threads for
these can be joined.

(T2) We can discard the results of a task. When we're interrupting a
computation, such results won't be used. The file system tasks read and stat are
in this category, as are the web request and the body of the timeout task (i.e.
once it has finished waiting). These threads can be detached and we can rely on
the OS to clean up at exit. The engine destructor might not be called, but that
shouldn't matter because there will be no externally persistent effects.

(T3) We can cancel a task. The timeout task before its body has fired is in this
category. In this case there's no thread associated with the task.

https://codereview.adblockplus.org/29367003/diff/29367004/src/GlobalJsObject.cpp
File src/GlobalJsObject.cpp (left):

https://codereview.adblockplus.org/29367003/diff/29367004/src/GlobalJsObject....
src/GlobalJsObject.cpp:27: #include "ConsoleJsObject.h"
Duplicated.

https://codereview.adblockplus.org/29367003/diff/29367004/src/Thread.h
File src/Thread.h (left):

https://codereview.adblockplus.org/29367003/diff/29367004/src/Thread.h#oldcode31
src/Thread.h:31: class Mutex
Class was unused except in tests.

https://codereview.adblockplus.org/29367003/diff/29367004/src/Thread.h#oldcode46
src/Thread.h:46: class Lock
Class was unused except in tests.

https://codereview.adblockplus.org/29367003/diff/29367004/src/Thread.h#oldcode63
src/Thread.h:63: void Join();
Function was unused except in tests.

[sergei, 2017-01-16 14:50:13]

https://codereview.adblockplus.org/29367003/diff/29367004/src/Scheduler.cpp
File src/Scheduler.cpp (right):

https://codereview.adblockplus.org/29367003/diff/29367004/src/Scheduler.cpp#n...
src/Scheduler.cpp:36: auto th = std::thread(SingleUseThreadMain, task);
SingleUseThreadMain is definitely redundant in this commit.

https://codereview.adblockplus.org/29367003/diff/29367004/src/Scheduler.h
File src/Scheduler.h (right):

https://codereview.adblockplus.org/29367003/diff/29367004/src/Scheduler.h#new...
src/Scheduler.h:58: HeapFunction<T> MakeHeapFunction(std::shared_ptr<T> body)
It seems it can be achieved in a simpler way, e.g.

template<T>
std::function<void()> MakeHeapFunction(const std::shared_ptr<T>& obj) {
  return !!obj ? [obj]{ (*obj)(); } : function<void()>();
}

However, it's difficult to find a good name, so we can choose whatever.

https://codereview.adblockplus.org/29367003/diff/29367004/src/Scheduler.h#new...
src/Scheduler.h:75: static void
StartImmediatelyInSingleUseThread(std::function<void()> task);
It would be better to use const reference here.

[Eric, 2017-01-16 15:20:16]

All the suggestions here are about definitions that are eliminated in later
changes.

https://codereview.adblockplus.org/29367003/diff/29367004/src/Scheduler.cpp
File src/Scheduler.cpp (right):

https://codereview.adblockplus.org/29367003/diff/29367004/src/Scheduler.cpp#n...
src/Scheduler.cpp:36: auto th = std::thread(SingleUseThreadMain, task);
On 2017/01/16 14:50:13, sergei wrote:
> SingleUseThreadMain is definitely redundant in this commit.

In this commit, yes. It grows in later change sets.
StartImmediatelyInSingleUseThread is eliminated entirely a few changes hence.

If this were a final change it would be worth fixing.

https://codereview.adblockplus.org/29367003/diff/29367004/src/Scheduler.h
File src/Scheduler.h (right):

https://codereview.adblockplus.org/29367003/diff/29367004/src/Scheduler.h#new...
src/Scheduler.h:58: HeapFunction<T> MakeHeapFunction(std::shared_ptr<T> body)
On 2017/01/16 14:50:13, sergei wrote:
> It seems it can be achieved in a simpler way

Later on, the type HeapFunction gets used explicitly.

And later than that HeapFunction and MakeHeapFunction go away entirely. They're
only used with detached threads, and those have been eliminated.

https://codereview.adblockplus.org/29367003/diff/29367004/src/Scheduler.h#new...
src/Scheduler.h:75: static void
StartImmediatelyInSingleUseThread(std::function<void()> task);
On 2017/01/16 14:50:13, sergei wrote:
> It would be better to use const reference here.

It's a transient function. We can change it, but it's make-work. It's already
eliminated in later change.

[sergei, 2017-01-17 08:41:02]

On 2017/01/16 15:20:16, Eric wrote:
> All the suggestions here are about definitions that are eliminated in later
> changes.
However, later changes will be later. Why not to create a better commit now?
const reference for task argument is not so important but SingleUseThreadMain
and HeapFunction definitely should not be in the commit.

[Eric, 2017-01-17 13:37:13]

On 2017/01/17 08:41:02, sergei wrote:
> However, later changes will be later.

No, the later changes are already here. This is the first of a sequence of 9
reviews that started 5 weeks ago.

> Why not to create a better commit now?

1) Because it's a lot of textual manipulation for no real benefit. Lots of hours
for, in the end, nothing.

2) The rebase operation can introduce defects. It's even more hours than just
the text manipulation to retest everything.

Neither of the things you want changed are defects. I'll care about that. I
cannot bring myself to care about style or efficiency concerns in code that's
already been removed.

I think the best thing to do would be (1) to commit all this new code to a
branch, (2) save up style and efficiency concerns in a tracking issue for the
branch, (3) fix all the outstanding points at the end, and finally (4) merge
back a clean commit.

[sergei, 2017-01-17 13:40:54]

I hope I have not missed anything in the rest reviews.

There is no usage of HeapFunction class, makeHeapFunction creates it and we
immediately construct function<void()> from it (why not to simply construct
function?) and in the last review this code is removed. I think that not
introducing HeapFunction will simplify the code and reduce the number of
changes. Similar with SingleUseThreadMain, as I see it's not used anywhere else
and finally is removed, it's used only by StartImmediatelyInSingleUseThread
which has no changes and it is finally removed. Why do we need to construct
function from SingleUseThreadMain function, which accepts function and calls it,
and argument for it , when we have already the function which can be directly
passed into std::thread constructor?

I guess rebasing also shouldn't be difficult, except the reviews where it's
introduced and removed there is one codereview with src/Scheduler.cpp two
codereviews with src/Scheduler.h, and the changes are not close to the code,
mentioned above, so even if a conflict appears there, it should be trivial to
resolve it.

[Eric, 2017-01-17 13:58:09]

On 2017/01/17 13:40:54, sergei wrote:
> I think that not
> introducing HeapFunction will simplify the code and reduce the number of
> changes.

It will increase the number of changes, because it means changing code that
already in review for no effect at the end of it.

> Similar with SingleUseThreadMain, as I see it's not used anywhere else
> and finally is removed

Please focus on the last bit "finally is removed". If we were still using it at
the end of all of this, what you're saying would matter. It's gone soon, and so
its intermediate quality matters not in the slightest.

> I guess rebasing also shouldn't be difficult, except the reviews where it's
> introduced and removed there is one codereview with src/Scheduler.cpp two
> codereviews with src/Scheduler.h, and the changes are not close to the code,
> mentioned above, so even if a conflict appears there, it should be trivial to
> resolve it.

I'm so glad you're volunteering to do the work then, since it's so trivial. How
might I get you copy of all the changes?

I'm going to repeat the following, because I want you to take this suggestion
seriously:

I think the best thing to do would be (1) to commit all this new code to a
branch, (2) save up style and efficiency concerns in a tracking issue for the
branch, (3) fix all the outstanding points at the end, and finally (4) merge
back a clean commit.

[sergei, 2017-01-17 14:04:31]

On 2017/01/17 13:58:09, Eric wrote:
...
> I'm going to repeat the following, because I want you to take this suggestion
> seriously:
> 
> I think the best thing to do would be (1) to commit all this new code to a
> branch, (2) save up style and efficiency concerns in a tracking issue for the
> branch, (3) fix all the outstanding points at the end, and finally (4) merge
> back a clean commit.
LGTM if it goes into branch (pay attention we use bookmarks in mercurial).

[Eric, 2017-01-17 15:25:49]

On 2017/01/17 14:04:31, sergei wrote:
> LGTM if it goes into branch (pay attention we use bookmarks in mercurial).

Branch created. Bookmark "master" left where it was.

Added tracking issue #4811 for changes deferred to the end of the branch that
will be expedient to deal with outside of a specific change set.

[sergei, 2017-01-17 15:30:10]

On 2017/01/17 15:25:49, Eric wrote:
> On 2017/01/17 14:04:31, sergei wrote:
> > LGTM if it goes into branch (pay attention we use bookmarks in mercurial).
> 
> Branch created. Bookmark "master" left where it was.
OK. Could you please create bookmark instead of branch and push all reviews?

> 
> Added tracking issue #4811 for changes deferred to the end of the branch that
> will be expedient to deal with outside of a specific change set.

[Eric, 2017-01-17 15:38:52]

> On 2017/01/17 15:25:49, Eric wrote:
> > Branch created. Bookmark "master" left where it was.

On 2017/01/17 15:30:10, sergei wrote:
> OK. Could you please create bookmark instead of branch and push all reviews?

I think it's better to use a heavyweight named branch for this one. That's what
I just pushed to the repository.

If you *really* want this pushed to the default branch instead, I'll merge it
and add a bookmark.

I can push all the reviews, certainly, but I'd greatly prefer to push
code-in-review to a proper branch rather than merely having it bookmarked.

[Eric, 2017-01-17 15:41:25]

P.S. I think it will go much faster if I do push all the reviews. So let's
decide how to do that.

Another thing we might do is to make a "summary review" of all the changes, not
to commit as a collapsed change set, but to see everything in one place.

[sergei, 2017-01-17 15:59:33]

On 2017/01/17 15:38:52, Eric wrote:
> > On 2017/01/17 15:25:49, Eric wrote:
> > > Branch created. Bookmark "master" left where it was.
> 
> On 2017/01/17 15:30:10, sergei wrote:
> > OK. Could you please create bookmark instead of branch and push all reviews?
> 
> I think it's better to use a heavyweight named branch for this one. That's
what
> I just pushed to the repository.
Mercurial branches are not mapped to git,
https://github.com/adblockplus/libadblockplus/branches.
> 
> If you *really* want this pushed to the default branch instead, I'll merge it
> and add a bookmark.

> P.S. I think it will go much faster if I do push all the reviews. So let's
decide how to do that.

I think the best way would be to simply create a bookmark in default branch and
push all changes in default branch and with the bookmark.
(I don't know how it works internally, maybe simply adding of a bookmark in that
branch will also trigger the synchronization to git. And I don't think it should
be merged into default branch.).

> 
> I can push all the reviews, certainly, but I'd greatly prefer to push
> code-in-review to a proper branch rather than merely having it bookmarked.

> Another thing we might do is to make a "summary review" of all the changes,
not
to commit as a collapsed change set, but to see everything in one place.
I don't think it is worth doing right now. BTW, it will be possible to see all
cumulative changes on github if one chooses create a pull request (AFAIR, it's
possible only in a personal repo, either fork or just a repo), it's not
necessary to actually create it.

[Eric, 2017-01-17 16:10:14]

On 2017/01/17 15:59:33, sergei wrote:
> Mercurial branches are not mapped to git,
> https://github.com/adblockplus/libadblockplus/branches.

Exactly. I don't think we want code-under-review to be pushed to git. We
certainly don't need it pushed there to accomplish all the reviewing.

> I think the best way would be to simply create a bookmark in default branch
and
> push all changes in default branch and with the bookmark.
> (I don't know how it works internally, maybe simply adding of a bookmark in
that
> branch will also trigger the synchronization to git. And I don't think it
should
> be merged into default branch.).

It's out of my pay grade to decide whether we need the whole commit history in
git or not. That, however, is a decision we can make later. If we want the whole
history in git, we can graft it in mercurial and it will get copied over. If a
summary in git is OK, then a merge in mercurial will collapse the branch to a
single change which gets mirrored to git.

In either case, however, we can work with a named branch for now.

> I don't think it is worth doing right now. BTW, it will be possible to see all
> cumulative changes [...] it's not
> necessary to actually create it.

I was thinking of it not so much for simple visibility, which a commit would do
for, but rather as a place to identify items for end-of-branch work (#4811).

[Eric, 2017-01-19 15:33:32]

Still awaiting response about whether to commit the branch this change set
begins on (1) (my preference) to a named branch or (2) to the default branch
with a bookmark.

[Felix Dahlke, 2017-01-19 15:36:00]

On 2017/01/19 15:33:32, Eric wrote:
> Still awaiting response about whether to commit the branch this change set
> begins on (1) (my preference) to a named branch or (2) to the default branch
> with a bookmark.

Haven't read up on the entire discussion, but we generally use bookmarks for
branching rather than named branches because it enables Git interoperability. So
bookmark please :)

[Eric, 2017-01-19 16:06:41]

On 2017/01/19 15:36:00, Felix Dahlke wrote:
> Haven't read up on the entire discussion

Ah, well, it's important this time. I'll summarize.

> but we generally use bookmarks for
> branching rather than named branches because it enables Git interoperability.
So
> bookmark please :)

Sergei asked for a commit of a large branch of development (6 weeks or so of
mine) that is still under review. It's the branch that fixes #3593, gets the
isolate under control, tames the threading (no more detaching), and allows the
unit tests to run without a shared isolate. See
https://codereview.adblockplus.org/29371607/ for what (aside from a fixup change
set) should be end of the branch.

It's definitely the easiest way to deal with this, since the change in total
really is too large for a single review here. I suggested a named branch, in
part because it *will not* be mirrored to git. Sergei (and you, Felix) are the
only audience for the branch, and it's life span is only that of this review
cycle. It's a way of accelerating the review process for this particular
circumstance, not a new ordinary policy.

We can graft over individual changes at the end to a branch that *will* be
mirrored to git at the end of all this.

[Felix Dahlke, 2017-01-19 16:30:50]

On 2017/01/19 16:06:41, Eric wrote:
> On 2017/01/19 15:36:00, Felix Dahlke wrote:
> > Haven't read up on the entire discussion
> 
> Ah, well, it's important this time. I'll summarize.
> 
> > but we generally use bookmarks for
> > branching rather than named branches because it enables Git
interoperability.
> So
> > bookmark please :)
> 
> Sergei asked for a commit of a large branch of development (6 weeks or so of
> mine) that is still under review. It's the branch that fixes #3593, gets the
> isolate under control, tames the threading (no more detaching), and allows the
> unit tests to run without a shared isolate. See
> https://codereview.adblockplus.org/29371607/ for what (aside from a fixup
change
> set) should be end of the branch.
> 
> It's definitely the easiest way to deal with this, since the change in total
> really is too large for a single review here. I suggested a named branch, in
> part because it *will not* be mirrored to git. Sergei (and you, Felix) are the
> only audience for the branch, and it's life span is only that of this review
> cycle. It's a way of accelerating the review process for this particular
> circumstance, not a new ordinary policy.
> 
> We can graft over individual changes at the end to a branch that *will* be
> mirrored to git at the end of all this.

Hm, for this use case I'd actually suggest option three: A separate clone on
BitBucket :) There it doesn't matter much whether a named branch or a bookmarked
one is used. And that way there's no permanent trace of this work in the
upstream repo. I personally do this regularly, I keep forks of all repos i work
on in my own BitBucket account and changes in feature branches there. After
review, I merge them all as a single commit.

[hub, 2017-05-12 14:45:40]

Eric,

We would really like to move forward and merge this (and other changes) in the
master branch.

Master has evolved a bit in since the original submission and this patch will
need a probably non trivial rebasing. Recent changes in master include getting
rid of JsValuePtr and other Ptr, changes in the tests, etc.

First an foremost, you need to move out the changes for replacing
AdblockPlus::Sleep() with std::this_thread::sleep_for() into a different patch.
This is really a standalone change and we'll review it quickly to get it in
first.

For the main part, on the review request, a few things need to be addressed. As
it was already outlined, removing bits that are not needed at the present time
would help make the changeset smaller. The smaller, the easier it will be to
move forward with this and the other changes you have been waiting for.

See my inline comments. 

Let me know if you have questions or need any help. Thanks!

https://codereview.adblockplus.org/29367003/diff/29367004/src/Scheduler.cpp
File src/Scheduler.cpp (right):

https://codereview.adblockplus.org/29367003/diff/29367004/src/Scheduler.cpp#n...
src/Scheduler.cpp:36: auto th = std::thread(SingleUseThreadMain, task);
With the other suggestion with task that is a unique_ptr<T>, you could just do 

// can't capture a unique_ptr<> in a lambda
std::shared_ptr<T> pt(std::move(task));
auto th = std::thread([pt]() { (*pt)(); });

https://codereview.adblockplus.org/29367003/diff/29367004/src/Scheduler.h
File src/Scheduler.h (right):

https://codereview.adblockplus.org/29367003/diff/29367004/src/Scheduler.h#new...
src/Scheduler.h:58: HeapFunction<T> MakeHeapFunction(std::shared_ptr<T> body)
If this is needed for something we want to remove, why the change? You can still
enforce the "single use" semantics with the move semantics and unique_ptr<>.
(based on how it is used in this changeset)

https://codereview.adblockplus.org/29367003/diff/29367004/src/Scheduler.h#new...
src/Scheduler.h:75: static void
StartImmediatelyInSingleUseThread(std::function<void()> task);
Here we could just take a std::unique_ptr<T>&& as a parameter, and what's above
is no longer needed. (T is a template parameter, since there is no more base
class of the "task")