Issue #3595 - Add an actual scheduler; use joined threads for file system

include/AdblockPlus/JsEngine.h

 /*
  * This file is part of Adblock Plus <https://adblockplus.org/>,
  * Copyright (C) 2006-2016 Eyeo GmbH
  *
  * Adblock Plus is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License version 3 as
  * published by the Free Software Foundation.
  *
  * Adblock Plus is distributed in the hope that it will be useful,
  * but WITHOUT ANY WARRANTY; without even the implied warranty of
  * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
  * GNU General Public License for more details.
  *
  * You should have received a copy of the GNU General Public License
  * along with Adblock Plus.  If not, see <http://www.gnu.org/licenses/>.
  */
 
 #ifndef ADBLOCK_PLUS_JS_ENGINE_H
 #define ADBLOCK_PLUS_JS_ENGINE_H
 
 #include <functional>
 #include <map>
+#include <memory>
 #include <stdexcept>
 #include <stdint.h>
 #include <string>
 #include <AdblockPlus/AppInfo.h>
 #include <AdblockPlus/LogSystem.h>
 #include <AdblockPlus/FileSystem.h>
 #include <AdblockPlus/JsValue.h>
 #include <AdblockPlus/WebRequest.h>
 
 namespace v8
@@ skipping 33 matching lines @@
     ScopedV8Isolate& operator=(const ScopedV8Isolate&);
 
     v8::Isolate* isolate;
   };
 
   /**
    * Shared smart pointer to ScopedV8Isolate instance;
    */
   typedef std::shared_ptr<ScopedV8Isolate> ScopedV8IsolatePtr;
 
+  struct ImmediateSingleUseThreadType {}; ///< Marker class for scheduling policy
+  extern const ImmediateSingleUseThreadType ImmediateSingleUseThread; ///< Dummy constant for scheduling policy

[sergei, 2017/01/20 13:08:13]

Since it's not used I would propose to remove any ImmediateSingleUseThread*.

+
   /**
    * JavaScript engine used by `FilterEngine`, wraps v8.
    */
   class JsEngine : public std::enable_shared_from_this<JsEngine>
   {
     friend class JsValue;
     friend class JsContext;
 
   public:
     /**
@@ skipping 40 matching lines @@
      * Evaluates a JavaScript expression.
      * @param source JavaScript expression to evaluate.
      * @param filename Optional file name for the expression, used in error
      *        messages.
      * @return Result of the evaluated expression.
      */
     JsValuePtr Evaluate(const std::string& source,
         const std::string& filename = "");
 
     /**
+     * Schedule a task with these policies:
+     * - timing policy: start execution immediately.

[sergei, 2017/01/20 13:08:12]

I would remove the comment about timing policy because no one can guarantee that
execution will be started immediately.

[Eric, 2017/03/30 17:16:58]

"Immediately" simply means "at the present time, without delay".

I would have thought it was obvious that this was not an assertion about the
real-time behavior of execution, since we are not in an RTOS (real-time
operating system) environment, but apparently not.

+     * - threading policy: run the task in a single-use thread.
+     *
+     * @param task
+     *   Task to execute. All v8 handles should refer to the present engine.
+     * @param ImmediateSingleUseThreadType
+     *   The schedule policy--create a new thread and discard it afterwards.

[sergei, 2017/01/20 13:08:12]

I would remove that parameter because there is no choice and this parameters has
no influence.

[Eric, 2017/03/30 17:16:58]

This parameter is a marker, acting both as documentation and disambiguation by
function signature. There's not a second version of `Schedule()` in the present
code, but there has been in development code. When I've rewritten this function,
I've created a second marker class and another implementation and then moved
over callers from old to new incrementally, which greatly speeds development.
And because it's a parameter, not a new function name, it play well with
template instantiation.

Once the scheduler can be considered finished, then it will be time to get rid
of this argument. Even in the initial series of 12 review, it wasn't time yet.

+     */
+    void Schedule(std::function<void()> task, ImmediateSingleUseThreadType);

[sergei, 2017/01/20 13:08:13]

Although it's expected that task can be copied internally, it would be still
better to pass it as const reference.

[Eric, 2017/03/30 17:16:58]

In the present code, it's passed by value intentionally. We have to invoke the
copy constructor because we'll eventually run it out of scope, and the cleanest
way of doing this is to do it as part of the call.

In later code, I changed this to an rvalue argument. The reason for that is to
put responsibility for allocation inside the scheduler instead of relying upon
its callers to do the right thing. The caller creates an argument and calls it
with std::move. If I had figured this out before, I would have done it that way
from the outset.

[sergei, 2017/04/03 15:35:31]

Acknowledged.

+
+    /**
+     * Block until there are no more tasks running in the scheduler.
+     *
+     * Note: this function does not prevent new tasks from being scheduled.
+     */
+    void WaitForQuietScheduler();
+
+    /**
      * Initiates a garbage collection.
      */
     void Gc();
 
     //@{
     /**
      * Creates a new JavaScript value.
      * @param val Value to convert.
      * @return New `JsValue` instance.
      */
@@ skipping 111 matching lines @@
 
     /// Isolate must be disposed only after disposing of all objects which are
     /// using it.
     ScopedV8IsolatePtr isolate;
 
     FileSystemPtr fileSystem;
     WebRequestPtr webRequest;
     LogSystemPtr logSystem;
     std::unique_ptr<v8::Persistent<v8::Context>> context;
     EventMap eventCallbacks;
+
+    // Forward declaration for PImpl idiom

[sergei, 2017/01/20 13:08:13]

We don't need this comment.

[Eric, 2017/03/30 17:16:58]

Yes, we do. Dangling declarations with zero documentation are always confusing.

+    class SchedulerImpl;
+    /**
+     * Scheduler for tasks executed under the current engine.
+     *
+     * \par Invariant
+     *   `bool(scheduler)` is always true
+     */
+    std::unique_ptr<SchedulerImpl> scheduler;
   };
 }
 
 #endif