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

src/JsContext.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_CONTEXT_H
 #define ADBLOCK_PLUS_JS_CONTEXT_H
 
 #include <v8.h>
 #include <AdblockPlus/JsEngine.h>
 
 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
     {
       return context;
     }
 
   private:
-    const v8::Locker locker;
+    const JsLocker locker;
     const v8::Isolate::Scope isolateScope;
     const v8::HandleScope handleScope;
     const v8::Local<v8::Context> context;
     const v8::Context::Scope contextScope;
   };
 }
 
 #endif