Issue 29377060: Noissue - add JsLocker which is merely v8::Locker but can be used with our JsEngine. (Closed)
|
Created:
Feb. 28, 2017, 12:01 p.m. by sergei Modified:
Feb. 28, 2017, 10:45 p.m. CC:
anton Visibility:
Public. |
DescriptionIt's useful as a synchronization primitive in JsEngine and it will be used in next commits.
Patch Set 1 #
Total comments: 6
MessagesTotal messages: 8
One nit. Otherwise LGTM. https://codereview.adblockplus.org/29377060/diff/29377061/src/JsContext.h File src/JsContext.h (right): https://codereview.adblockplus.org/29377060/diff/29377061/src/JsContext.h#new... src/JsContext.h:26: class JsLocker { Nit: JsLocker is pretty ambivalent. How about JsEngineLocker?
https://codereview.adblockplus.org/29377060/diff/29377061/src/JsContext.h File src/JsContext.h (right): https://codereview.adblockplus.org/29377060/diff/29377061/src/JsContext.h#new... src/JsContext.h:26: class JsLocker { 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. https://codereview.adblockplus.org/29377060/diff/29377061/src/JsContext.h#new... src/JsContext.h:36: explicit JsContext(const JsEnginePtr& jsEngine); 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.
https://codereview.adblockplus.org/29377060/diff/29377061/src/JsContext.h File src/JsContext.h (right): https://codereview.adblockplus.org/29377060/diff/29377061/src/JsContext.h#new... src/JsContext.h:26: class JsLocker { On 2017/02/28 16:26:44, Eric wrote: > 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. 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.
On 2017/02/28 17:33:56, Oleksandr wrote: >provided this is actually needed. I don't believe it's needed. The review where it's used doesn't need any C++ code at all; it can all be done in the JS code.
https://codereview.adblockplus.org/29377060/diff/29377061/src/JsContext.h File src/JsContext.h (right): https://codereview.adblockplus.org/29377060/diff/29377061/src/JsContext.h#new... src/JsContext.h:26: class JsLocker { On 2017/02/28 16:26:44, Eric wrote: > 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. 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. https://codereview.adblockplus.org/29377060/diff/29377061/src/JsContext.h#new... src/JsContext.h:36: explicit JsContext(const JsEnginePtr& jsEngine); On 2017/02/28 16:26:44, Eric wrote: > 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, It seems to be the reason you are always missing const reference :). > 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. 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. > > 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.
Message was sent while issue was closed.
On 2017/02/28 22:14:48, sergei wrote: > Just wonder, could you give an example of a contemporary implementation of > shared_ptr based on mutexes? Insofar as I know, most are. VS 2015 is, I'm pretty sure. Not on std::mutex necessarily, but on some kind of multiprocessing primitive. It's a requirement of the standard that std::shared_ptr work in a multithreaded environment without any special declaration. It was a debate during standardization whether they might have some policy argument or something that was a single-threaded version, but consistency of use was thought to be superior. Some newer compilers are using lock-free implementations, but that's a relatively recent development. > 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. Passing a raw pointer is even better unless you have a specific need to manage the lifespan of an argument past the scope of the function. Passing around shared_ptr because you don't want to think about life cycle issues just leads you down the path to self-reference and memory leaks.
Message was sent while issue was closed.
On 2017/02/28 22:30:19, Eric wrote: > On 2017/02/28 22:14:48, sergei wrote: > > Just wonder, could you give an example of a contemporary implementation of > > shared_ptr based on mutexes? > > Insofar as I know, most are. VS 2015 is, I'm pretty sure. I'm absolutely sure that it's not the case because I have looked into the code it. > Not on std::mutex necessarily, But the question was about mutex-based because lock-free and lock-based are different worlds. > but on some kind of multiprocessing primitive. It's a requirement > of the standard that std::shared_ptr work in a multithreaded environment without > any special declaration. It was a debate during standardization whether they > might have some policy argument or something that was a single-threaded version, > but consistency of use was thought to be superior. > > Some newer compilers are using lock-free implementations, but that's a > relatively recent development. It depends how to define "recent". > > > 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. > > Passing a raw pointer is even better unless you have a specific need to manage > the lifespan of an argument past the scope of the function. The usage of references is considered to be safer than usage of raw pointers and I agree with it. > Passing around > shared_ptr because you don't want to think about life cycle issues just leads > you down the path to self-reference and memory leaks. Yes, but it's not related to the statement above. 
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
