Noissue - add JsLocker which is merely v8::Locker but can be used with our JsEngine.

src/JsContext.h

Index: src/JsContext.h
diff --git a/src/JsContext.h b/src/JsContext.h
index 1a00107d0662d5fb8e86b68d20447703c29925d5..605ed8bf2b5b0048a67eb480a6c88914a0a5c9dc 100644
--- a/src/JsContext.h
+++ b/src/JsContext.h
@@ -23,10 +23,17 @@
 
 namespace AdblockPlus
 {
+  class JsLocker {

[Oleksandr, 2017/02/28 15:36:02]

Nit: JsLocker is pretty ambivalent. How about JsEngineLocker?

[Eric, 2017/02/28 16:26:44]

I've been deep in this code, and I don't see why this is needed. Indeed it seems
harmful to the API, since it means inducing a requirement for users of the API
to be aware of multiprocessing state, which is a complication to the API.

I resolved a number of problems when I was rewriting the I/O code. Nothing there
required changing the API to increase its complexity.

[Oleksandr, 2017/02/28 17:33:56]

To clarify, this looks good to me, provided this is actually needed. I assume
the need is in one of these reviews:
https://codereview.adblockplus.org/29377064/ or 
https://codereview.adblockplus.org/29377570/. I am still reviewing those.

[sergei, 2017/02/28 22:14:48]

I used it to synchronize access to JsEngine::isConnectionAllowed. However,
currently indeed it seems better to just add a mutex to protect
JsEngine::isConnectionAllowed. I am closing this codereview.

+  public:
+    explicit JsLocker(const JsEnginePtr& jsEngine);
+  private:
+    const v8::Locker locker;
+  };
+
   class JsContext
   {
   public:
-    explicit JsContext(const JsEnginePtr jsEngine);
+    explicit JsContext(const JsEnginePtr& jsEngine);

[Eric, 2017/02/28 16:26:44]

Changing this API signature is not documented in the commit message.

This change is a step backward in performance, since it means copying a
shared_ptr, which means manipulating mutexes under the hood. You get rid of a
double-indirection in generated code, but you're paying for it with something
more expensive.

I'd recommend not making this change at the current time. There's a decent way
to do it, but it requires a language feature that we don't have with VS 2012,
namely with a delegating constructor. Add another constructor with argument
"const JsEngine*" and make it the ultimate constructor. Then change the current
constructor to be inline, to call get() on the reference, and then delegate.
This would get rid of the double indirection without the overhead of a
shared_ptr copy.

[sergei, 2017/02/28 22:14:48]

It seems to be the reason you are always missing const reference :).
Just wonder, could you give an example of a contemporary implementation of
shared_ptr based on mutexes?

JIC, to clarify, I think that if there is no necessity then std::shared_ptr
should be passed by reference because "atomic" operations do have a considerable
cost. Although we might cannot notice the benefit from it now but it's
definitely a good practice to have.

 
     v8::Local<v8::Context> GetV8Context() const
     {
@@ -34,7 +41,7 @@ namespace AdblockPlus
     }
 
   private:
-    const v8::Locker locker;
+    const JsLocker locker;
     const v8::Isolate::Scope isolateScope;
     const v8::HandleScope handleScope;
     const v8::Local<v8::Context> context;