Issue 6233220328718336: Issue #3593, #1197- fix isolate management (Closed)
DescriptionPlease consider this change as a commit in the branch "issue-1197". In new v8 v8::Isolate does not get instantiated automatically (inside `v8::Isolate::GetCurrent()`) anymore, so we have to manually call the creator method (`v8::Isolate::New()`) as well as it addresses the issue to free it when we don't need it.
The proposed change does work with current version of v8 as well as with a newer one.
The hack with `createJsEngine` is required for both versions.
It seems current version of v8 does not permit our approach in tests (create new Isolate in each JsEngine instance). I would suppose it's because the previous isolate "stored" in TLS is still "alive" but we are creating a new one.
Newer v8 is trying to avoid using of `v8::Isolate::GetCurrent` but despite of it, tests are still failing because starting from some point v8 cannot obtain more memory.
I think it's mainly because calls of setTimeout create background threads which keep JS engine from destroying and we consume all 2GB (available by default on 32bit Windows).
# depends on https://codereview.adblockplus.org/5598762307158016/
Patch Set 1 #
Total comments: 12
Patch Set 2 : fix the crash (hacky) #Patch Set 3 : rebase only #Patch Set 4 : make ScopedV8Isolate uncopyable #
Total comments: 32
Patch Set 5 : address comments #
Total comments: 14
Patch Set 6 : fix default arg for JsEngine::New #Patch Set 7 : fix (c) year #
Total comments: 2
Patch Set 8 : fix typo #
Total comments: 2
Patch Set 9 : rebase fix v8 initialization #
MessagesTotal messages: 32
Simplifying this change just to initialization order may well let this test pass. https://codereview.adblockplus.org/6233220328718336/diff/5629499534213120/inc... File include/AdblockPlus/JsEngine.h (left): https://codereview.adblockplus.org/6233220328718336/diff/5629499534213120/inc... include/AdblockPlus/JsEngine.h:220: v8::Isolate* isolate; Declared fourth, not first. Just fix it here. https://codereview.adblockplus.org/6233220328718336/diff/5629499534213120/inc... File include/AdblockPlus/JsEngine.h (right): https://codereview.adblockplus.org/6233220328718336/diff/5629499534213120/inc... include/AdblockPlus/JsEngine.h:58: class ScopedV8Isolate There's no need for this class. Member construction order is declaration-from-first-to-last and destruction order is declaration-from-last-to-first. Simply declare it first in 'JsEngine'. See, for example, http://stackoverflow.com/questions/2254263/order-of-member-constructor-and-de... which quotes the standard. Note: construction order is not specified by the order of initializers in the construction declaration. The rationale for this is that there can be only one destructor, no matter how the object is constructed. For consistency, all the constructors must initialize in the same order. https://codereview.adblockplus.org/6233220328718336/diff/5629499534213120/inc... include/AdblockPlus/JsEngine.h:72: */ Keep this comment (after a rewrite) at the declaration of the isolate pointer, and at its new location. It would be easy to rearrange the declaration order (for some other reason) unless the rationale is recorded. https://codereview.adblockplus.org/6233220328718336/diff/5629499534213120/src... File src/JsEngine.cpp (right): https://codereview.adblockplus.org/6233220328718336/diff/5629499534213120/src... src/JsEngine.cpp:69: : isolate(v8::Isolate::New()) This is changing the behavior non-trivially, since initializing v8 creates an isolate by default. Calling 'New()' here would (I think) abandon the default instance. Regardless, initialization policy seems like a different issue from getting order of initialization right. https://codereview.adblockplus.org/6233220328718336/diff/5629499534213120/src... src/JsEngine.cpp:75: isolate->Dispose(); If we initialize with 'New()' rather than 'GetCurrent()', then we need to call 'Dispose()'. Otherwise we don't. Even if we do need it, we don't need it in a destructor of a separate class. v8, as a Google creature, doesn't do RAII or throw its own exceptions.
https://codereview.adblockplus.org/6233220328718336/diff/5629499534213120/inc... File include/AdblockPlus/JsEngine.h (right): https://codereview.adblockplus.org/6233220328718336/diff/5629499534213120/inc... include/AdblockPlus/JsEngine.h:58: class ScopedV8Isolate On 2015/08/05 22:29:16, Eric wrote: > There's no need for this class. Member construction order is > declaration-from-first-to-last and destruction order is > declaration-from-last-to-first. Simply declare it first in 'JsEngine'. > > See, for example, > http://stackoverflow.com/questions/2254263/order-of-member-constructor-and-de... > which quotes the standard. > > Note: construction order is not specified by the order of initializers in the > construction declaration. The rationale for this is that there can be only one > destructor, no matter how the object is constructed. For consistency, all the > constructors must initialize in the same order. Thanks for explanation but this class and construction order are not related. This class is aimed to manage `v8::Isolate`. https://codereview.adblockplus.org/6233220328718336/diff/5629499534213120/inc... include/AdblockPlus/JsEngine.h:72: */ On 2015/08/05 22:29:16, Eric wrote: > Keep this comment (after a rewrite) at the declaration of the isolate pointer, > and at its new location. It would be easy to rearrange the declaration order > (for some other reason) unless the rationale is recorded. I've inherited because it's more reliable than the order of members. It looks like there is no any convention and it's very easy to get something using `v8::Isolate` accidentally declared before isolate manager. Now it's a member, because it's injected in the constructor. https://codereview.adblockplus.org/6233220328718336/diff/5629499534213120/src... File src/JsEngine.cpp (right): https://codereview.adblockplus.org/6233220328718336/diff/5629499534213120/src... src/JsEngine.cpp:69: : isolate(v8::Isolate::New()) On 2015/08/05 22:29:16, Eric wrote: > This is changing the behavior non-trivially, since initializing v8 creates an > isolate by default. Calling 'New()' here would (I think) abandon the default > instance. There is no any default isolate. It `v8::Isolate::GetCurrent` returns the isolate, pointer on which is stored thread local storage. > > Regardless, initialization policy seems like a different issue from getting > order of initialization right. Sorry, I don't understand about which issues you are talking here. The changes here are part of updating of v8. In newer v8 one may not use `v8::Isolate::GetCurrent()` when isolate is not initialized yet, so we need to create it. https://codereview.adblockplus.org/6233220328718336/diff/5629499534213120/src... src/JsEngine.cpp:75: isolate->Dispose(); On 2015/08/05 22:29:16, Eric wrote: > If we initialize with 'New()' rather than 'GetCurrent()', then we need to call > 'Dispose()'. Otherwise we don't. > > Even if we do need it, we don't need it in a destructor of a separate class. v8, > as a Google creature, doesn't do RAII or throw its own exceptions. Do you mean we should create a typedef on `std::shared_ptr` for example? Google may be does not do RAII in v8 but it's a reason for us to avoid RAII.
https://codereview.adblockplus.org/6233220328718336/diff/5629499534213120/inc... File include/AdblockPlus/JsEngine.h (right): https://codereview.adblockplus.org/6233220328718336/diff/5629499534213120/inc... include/AdblockPlus/JsEngine.h:72: */ On 2015/11/16 16:52:09, sergei wrote: > I've inherited because it's more reliable than the order of members. Whatever other reason you might want for it, arguing on the grounds of reliability is a weak argument. Order of initialization is part of the C++ standard, and all major compilers are conformant. If you've got some other such reason, document it. What you have documented could be done better relying upon order of initialization. > It looks > like there is no any convention and it's very easy to get something using > `v8::Isolate` accidentally declared before isolate manager. "No convention"? If you mean convention for order of initialization, you are correct that it's not a convention, but rather a mandatory requirement for conformance with the standard. > Now it's a member, because it's injected in the constructor. I'm not claiming that the method here doesn't work. I am claiming that it is far more verbose than needed. https://codereview.adblockplus.org/6233220328718336/diff/5629499534213120/src... File src/JsEngine.cpp (right): https://codereview.adblockplus.org/6233220328718336/diff/5629499534213120/src... src/JsEngine.cpp:69: : isolate(v8::Isolate::New()) On 2015/11/16 16:52:09, sergei wrote: > There is no any default isolate. Sure there is. See isolate.cc:340 Isolate* Isolate::default_isolate_ = NULL; And also see isolate.c:396 void Isolate::EnsureDefaultIsolate() { In the current v8 code, each thread has a default isolate. When you call New() instead of GetCurrent(), you're changing behavior by discarding the default isolate that's initialized on the thread. After quite a bit of research, it seems that there's no longer a default isolate: https://code.google.com/p/chromium/issues/detail?id=359977 I'd suggest that a line of documentation is in order here. https://codereview.adblockplus.org/6233220328718336/diff/5629499534213120/src... src/JsEngine.cpp:75: isolate->Dispose(); On 2015/11/16 16:52:09, sergei wrote: > Do you mean we should create a typedef on `std::shared_ptr` for example? No. > Google may be does not do RAII in v8 but it's a reason for us to avoid RAII. And I'm not arguing against RAII. I am arguing that we don't need a separate class for a destructor to call Dispose() in. Such a destructor can be called in the destructor for JsEngine. My point about Google's practice is that Isolate::New() does not throw, so we can just initialize it in the constructor for JsEngine without any trouble.
I sent the previous batch of comments, which were all about previous comments, before I had read the new patch set. Having seen the new patch set, it doesn't seem to be a good idea to try to hack up a solution for the transition from GetCurrent() to New() that will span both versions of v8. It will be a lot cleaner just to make that change when we cut over to the new v8. Thus, I'd recommend splitting the original patch set into two: (1) the issues around order of initialization, which is immediately relevant, and (2) the change from GetCurrent() to New(). These can be done separately. Part (1) isn't even part of issue #1197, but is an unrelated defect; I don't know if it has issue in the tracker. Part (2) is related to #1197, but deserves its own issue. It wasn't at all obvious that GetCurrent() didn't work any more, and that's cause enough for its own issue.
On 2015/11/17 21:55:05, Eric wrote: > I sent the previous batch of comments, which were all about previous comments, > before I had read the new patch set. > > Having seen the new patch set, it doesn't seem to be a good idea to try to hack > up a solution for the transition from GetCurrent() to New() that will span both > versions of v8. It will be a lot cleaner just to make that change when we cut > over to the new v8. > > Thus, I'd recommend splitting the original patch set into two: (1) the issues > around order of initialization, which is immediately relevant, and (2) the > change from GetCurrent() to New(). These can be done separately. Part (1) isn't > even part of issue #1197, but is an unrelated defect; I don't know if it has > issue in the tracker. Part (2) is related to #1197, but deserves its own issue. > It wasn't at all obvious that GetCurrent() didn't work any more, and that's > cause enough for its own issue. Could you please explain a bit more, what you expect in patchset regarding issue around order of initialization? As I see now, it's merely a moving of `v8::Isolate* isolate;` member onto the first position, but it makes almost no sense to have it in a separate commit because it rather generates a lot of noise than doing something meaningful, the position of the pointer member which we don't touch in the destructor does not influence anything. Another option is to transform it into `ScopedV8IsolatePtr isolate;` (which obtains `v8::Isolate` using `v8::Isolate::GetCurrent()` and does nothing in the destructor) and move it on the first position, but in this case it will look like a adding of a buggy implementation of `ScopedV8IsolatePtr`, because it does not free the resource yet.
On 2015/11/17 21:55:05, Eric wrote: > Having seen the new patch set, it doesn't seem to be a good idea to try to hack > up a solution for the transition from GetCurrent() to New() that will span > both > versions of v8. It will be a lot cleaner just to make that change when we cut > over to the new v8. On 2016/01/22 10:41:04, sergei wrote: > Could you please explain a bit more, what you expect in patchset regarding issue > around order of initialization? Not today. I recall reading a lot of v8 code to be able to make those comments. Those comments languished for more than two months before you got around to replying. Frankly, I no longer recall all the reasons for my concerns. I'll have to go back and do the work again. I'm starting over on this review.
It is also necessary to know what version or versions of v8 this change set is targeting. The current description is inconclusive and ambiguous on this matter. If this change set is solely targeting a newer version of v8, then it makes a difference to the matter of what code is correct or not. In particular, I cannot distinguish between two situations. 1) This code is for current v8 and some future one 2) This code is for some future v8 only.
On 2016/01/25 14:13:01, Eric wrote: > It is also necessary to know what version or versions of v8 this change set is > targeting. The current description is inconclusive and ambiguous on this matter. > If this change set is solely targeting a newer version of v8, then it makes a > difference to the matter of what code is correct or not. Updated. > > In particular, I cannot distinguish between two situations. > 1) This code is for current v8 and some future one > 2) This code is for some future v8 only. The code is for v8 from #1197, it's because the issue is #1197 in the commit title. Fortunately, this logically separate change still works with the current version of v8, so it can be reviewed separately to simplify https://codereview.adblockplus.org/6193234183192576/.
My comments assume that the present change set will only be applied against a new version of v8. If there's a default isolate (as the current version), this code is suspect. I would not commit this code with the current version of v8, as it does not preserve current behavior. 1) What's currently 'ScopedV8Isolate' should really be a resource class 'V8Isolate'. Such a change would be simplify and clarify the code. 2) The modifications to the unit tests are an unrelated change. They should not be in this change set. It seems as though the change is for performance, but it's not documented. In any case, the unit tests can continue simply to call "JsEngine::New()" for the present change set. https://codereview.adblockplus.org/6233220328718336/diff/29334316/include/Adb... File include/AdblockPlus/JsEngine.h (right): https://codereview.adblockplus.org/6233220328718336/diff/29334316/include/Adb... include/AdblockPlus/JsEngine.h:53: * constructing and disposes it on destructing. Better wording in the comment would call this a "resource class". What's not obvious is that a 'v8::Isolate' does *not* act as a resource, and this class turns it into one. https://codereview.adblockplus.org/6233220328718336/diff/29334316/include/Adb... include/AdblockPlus/JsEngine.h:55: class ScopedV8Isolate Call it "V8Isolate". Every class is scoped; there's no need for that in the name. https://codereview.adblockplus.org/6233220328718336/diff/29334316/include/Adb... include/AdblockPlus/JsEngine.h:59: ~ScopedV8Isolate(); Since you're inlining the definition of "GetIsolate()", you might as well do so also for the constructor and destructor. https://codereview.adblockplus.org/6233220328718336/diff/29334316/include/Adb... include/AdblockPlus/JsEngine.h:65: v8::Isolate* isolate; This member should be private; the "protected" declaration had me looking for some derived class. https://codereview.adblockplus.org/6233220328718336/diff/29334316/include/Adb... include/AdblockPlus/JsEngine.h:74: typedef std::shared_ptr<ScopedV8Isolate> ScopedV8IsolatePtr; There's no need for this typedef, and 'shared_ptr' is not the right smart pointer. 1) There's only a single place this type is used; it's as a member of 'JsEngine'. There's no need for a typedef. But this observation is seeing a symptom of a larger problem. 2) We can use 'unique_ptr' to manage 'v8::Isolate'. 'JsEngine' is only ever accessed through a 'shared_ptr'. Sharing semantics happen through that type. There's no need to duplicate that behavior here. We don't have any code where one isolate is shared between many instance of 'JsEngine'. Using 'unique_ptr' is sufficient. 3) Allocation of 'v8::Isolate' should happen in 'class V8Isolate'. In the current patch set of the present review, the only way that this class is instantiated is by a call to 'make_shared'. There's no need to repeat the allocation everywhere it's used. That allocation can go in the constructor of 'V8Isolate' 4) The most recent patch set makes this class non-copyable. 'unique_ptr' is already non-copyable. Changing the isolate member from a raw pointer to 'unique_ptr' will be just fine. 5) 'V8Isolate::GetIsolate()' should call 'get()' on its underlying pointer; this is for clarity. And the name of this function can be just 'Get()' or even a conversion operator to the underlying pointer type. https://codereview.adblockplus.org/6233220328718336/diff/29334316/include/Adb... include/AdblockPlus/JsEngine.h:100: static JsEnginePtr New(const AppInfo& appInfo = AppInfo(), const ScopedV8IsolatePtr& isolate = ScopedV8IsolatePtr()); Having an argument for the isolate in the constructor seems like a Bad Idea. 'JsEngine' owns the isolate; it should be the one that creates it. It seems that the only reason to have an argument here is to support a shared isolate for the unit tests. I'm not even sure sharing isolates is a good idea, but if you need sharing for unit test, why not share a single instance of 'JsEngine'? https://codereview.adblockplus.org/6233220328718336/diff/29334316/include/Adb... include/AdblockPlus/JsEngine.h:246: v8::Isolate* GetIsolate() Note: returns a raw pointer, not a smart one, even though its member is a smart pointer. Better to call get() explicitly in such a case. https://codereview.adblockplus.org/6233220328718336/diff/29334316/src/JsEngin... File src/JsEngine.cpp (right): https://codereview.adblockplus.org/6233220328718336/diff/29334316/src/JsEngin... src/JsEngine.cpp:79: : isolate(isolate ? isolate : std::make_shared<ScopedV8Isolate>()) The hidden default in this initializer is both unnecessary and a bad idea. This constructor is only being called twice. 1) In 'CreateJsEngine()', where 'make_shared' is already being called. 2) in 'JsEngine()' right below, which relies on this default. Just pass the argument through. But, as I've said elsewhere, having an argument for the isolate here is a bad idea in the first place. https://codereview.adblockplus.org/6233220328718336/diff/29334316/test/BaseJs... File test/BaseJsTest.cpp (right): https://codereview.adblockplus.org/6233220328718336/diff/29334316/test/BaseJs... test/BaseJsTest.cpp:22: static AdblockPlus::ScopedV8IsolatePtr isolate = std::make_shared<AdblockPlus::ScopedV8Isolate>(); What's the motivation for using a single isolate for the tests? I can imagine that it's performance of the unit tests, but it doesn't say here. It _does_, however, make a difference to the semantics of the tests. By using a single isolate for all tests, it makes the tests interact with each other through potential side effects that a test leaves on the engine. That doesn't seem exactly desirable. If we're testing side effects, we should be doing so explicitly rather than implicitly through some artifact of the test harness. If performance is indeed the reason, and if it trumps isolation of the tests, then that trade-off should be documented here. In other words, some code change is needed here, even if it's only a comment. https://codereview.adblockplus.org/6233220328718336/diff/29334316/test/BaseJs... File test/BaseJsTest.h (right): https://codereview.adblockplus.org/6233220328718336/diff/29334316/test/BaseJs... test/BaseJsTest.h:141: AdblockPlus::JsEnginePtr createJsEngine(const AdblockPlus::AppInfo& appInfo = AdblockPlus::AppInfo()); According to our style guide, this should be "CreateJsEngine". https://codereview.adblockplus.org/6233220328718336/diff/29334316/test/BaseJs... test/BaseJsTest.h:141: AdblockPlus::JsEnginePtr createJsEngine(const AdblockPlus::AppInfo& appInfo = AdblockPlus::AppInfo()); In a suggestion that also appears elsewhere in a different way, the 'appInfo' argument could be omitted in favor of using application info that identifies the application as "unit tests". https://codereview.adblockplus.org/6233220328718336/diff/29334316/test/Filter... File test/FilterEngine.cpp (left): https://codereview.adblockplus.org/6233220328718336/diff/29334316/test/Filter... test/FilterEngine.cpp:87: appInfo.version = "1.0.1"; It would be a good idea to move this code into "CreateJsEngine()".
On 2016/01/25 14:13:01, Eric wrote: > In particular, I cannot distinguish between two situations. > 1) This code is for current v8 and some future one > 2) This code is for some future v8 only. On 2016/01/25 15:33:36, sergei wrote: > > The code is for v8 from #1197, it's because the issue is #1197 in the commit > title. This response answers nothing. I proffered two possibilities, both of which include the future version of v8. My question was whether this code would be applied *only* to a future version of v8 or not. > Fortunately, this logically separate change still works with the current > version of v8 I disagree with this. When you say "works", I agree it would compile and show initial behavior correctly. It does not preserve current behavior for the life cycle of the isolate, however. Upshot: I'll approve this code only for a future v8, not for a present one. I recommend making a branch for each v8 upgrade, since API changes are generally easier that way. When I spoke of splitting up the change set before, it is because changing the order of initialization would be sufficient to fix the defect in the issue description. Changing from 'GetCurrent()' to 'New()' would not be required.
On 2016/01/26 14:49:00, Eric wrote: > My comments assume that the present change set will only be applied against a > new version of v8. If there's a default isolate (as the current version), this > code is suspect. ... I have updated the description, now it explains why we need to manually instantiate it. > > 1) What's currently 'ScopedV8Isolate' should really be a resource class > 'V8Isolate'. Such a change would be simplify and clarify the code. Commented in the code. > > 2) The modifications to the unit tests are an unrelated change. They should not > be in this change set. It seems as though the change is for performance, but > it's not documented. In any case, the unit tests can continue simply to call > "JsEngine::New()" for the present change set. It does not address any performance issue, we have to use that hack to have working tests. > I would not commit this code with the current version of v8, as it does not preserve current behavior. How would you commit it then? I would like to remind that it's a separate commit because it is a logically grouped refactoring changes extracted to simplify another review and which do not break the current state of the library. Also I would like to remind that we are not using branches, how it's done in a git world, thus we don't group commits in a branch, each commit represents desirabely small amount of logically independent changes. As well as, JIC, it should not break anything. I hope if you answer yourself what is a refactoring commit you will understant the idea of the current change. On 2016/01/26 14:57:45, Eric wrote: > On 2016/01/25 14:13:01, Eric wrote: > > In particular, I cannot distinguish between two situations. > > 1) This code is for current v8 and some future one > > 2) This code is for some future v8 only. > > On 2016/01/25 15:33:36, sergei wrote: > > > > The code is for v8 from #1197, it's because the issue is #1197 in the commit > > title. > > This response answers nothing. I proffered two possibilities, both of which > include the future version of v8. My question was whether this code would be > applied *only* to a future version of v8 or not. > > > Fortunately, this logically separate change still works with the current > > version of v8 > > I disagree with this. When you say "works", I agree it would compile and show > initial behavior correctly. It does not preserve current behavior for the life > cycle of the isolate, however. > > Upshot: I'll approve this code only for a future v8, not for a present one. I > recommend making a branch for each v8 upgrade, since API changes are generally > easier that way. > > When I spoke of splitting up the change set before, it is because changing the > order of initialization would be sufficient to fix the defect in the issue > description. Changing from 'GetCurrent()' to 'New()' would not be required. Sorry, but I really don't see any relevant change in the order of initialization and consequently how it can help to figure out the defect explained in the issue description. Could you explain it? Secondly, this whole commit is caused by the change from 'GetCurrent()' to 'New()'. It's indeed required to update v8. I have added it into description. https://codereview.adblockplus.org/6233220328718336/diff/29334316/include/Adb... File include/AdblockPlus/JsEngine.h (right): https://codereview.adblockplus.org/6233220328718336/diff/29334316/include/Adb... include/AdblockPlus/JsEngine.h:53: * constructing and disposes it on destructing. On 2016/01/26 14:48:58, Eric wrote: > Better wording in the comment would call this a "resource class". What's not > obvious is that a 'v8::Isolate' does *not* act as a resource, and this class > turns it into one. It seems your terminology is slightly different from the popular one. v8::Isolate is indeed a resource and ScopedV8Isolate is a wrapper to manage its lifetime, in particular to initialize it and to release it using C++ concept of scopes. FYI: https://en.wikibooks.org/wiki/More_C%2B%2B_Idioms/Resource_Acquisition_Is_Ini... https://en.wikipedia.org/wiki/Resource_Acquisition_Is_Initialization https://codereview.adblockplus.org/6233220328718336/diff/29334316/include/Adb... include/AdblockPlus/JsEngine.h:55: class ScopedV8Isolate On 2016/01/26 14:48:58, Eric wrote: > Call it "V8Isolate". Every class is scoped; there's no need for that in the > name. I would not object to call it V8Isolate if it were an isolate in its essence, but here it's a wrapper around isolate because its single method is an accessor to the wrapped resource. It's common to describe the type of wrapper in its name, e.g. std::unique_ptr, std::shared_ptr, boost::scoped_ptr, in Qt - QScopedArrayPointer and so on. For example, we have JsValue which acts as JavaScript value in its essence, while the essence of wrapper (the accessor to internal resource) is used only by JsValue, so it's not ScopedJsValue. https://codereview.adblockplus.org/6233220328718336/diff/29334316/include/Adb... include/AdblockPlus/JsEngine.h:59: ~ScopedV8Isolate(); On 2016/01/26 14:48:58, Eric wrote: > Since you're inlining the definition of "GetIsolate()", you might as well do so > also for the constructor and destructor. Constructor and destructor are enough complicated methods which call some v8 methods. We usually don't declare third party library methods in headers because it's error-prone. It's also undesirable to include third party library headers in our headers, because in that case, the clients of our headers have to configure and care about too much stuff. Because of that the implementations of constructor and destructor are in c++ file that includes v8.h. `v8::Isolate` is also declared in this header (JsEngine.h), so we may not move the implementation of the getter which merely returns an opaque pointer to c++ file. https://codereview.adblockplus.org/6233220328718336/diff/29334316/include/Adb... include/AdblockPlus/JsEngine.h:65: v8::Isolate* isolate; On 2016/01/26 14:48:57, Eric wrote: > This member should be private; the "protected" declaration had me looking for > some derived class. Done. https://codereview.adblockplus.org/6233220328718336/diff/29334316/include/Adb... include/AdblockPlus/JsEngine.h:74: typedef std::shared_ptr<ScopedV8Isolate> ScopedV8IsolatePtr; On 2016/01/26 14:48:58, Eric wrote: > There's no need for this typedef, and 'shared_ptr' is not the right smart > pointer. > > 1) There's only a single place this type is used; it's as a member of > 'JsEngine'. There's no need for a typedef. But this observation is seeing a > symptom of a larger problem. Now it's used in several places in JsEngine and once in BaseJsTest.cpp, it's convenient to have a typedef for it. If it were used only a couple of times, there certainly would not be typedef for it. > > 2) We can use 'unique_ptr' to manage 'v8::Isolate'. 'JsEngine' is only ever > accessed through a 'shared_ptr'. Sharing semantics happen through that type. > There's no need to duplicate that behavior here. We don't have any code where > one isolate is shared between many instance of 'JsEngine'. Using 'unique_ptr' is > sufficient. std::unique_ptr with custom deleter requires to declare that deleter in the variable type, `std::unique_ptr<v8::Isolate, Deleter>`. So we have to either have a wrapper which calls `isolate->Dispose()` or use our own wrapper that does it in its destructor. v8::Isolate also requires a special initializer. Thus, having the wrapper around `v8::Isolate::Dispose` we spread the initialization and deinitialization around at least constructor of JsEngine and a deleter wrapper. I decided to create a special class whose responsibility is to initialize and deinitialize `v8::Isolate`, so we have them located in one unit. Having that I would like to have rather member ScopedV8Isolate than std::unique_ptr. As well as it allows to solve current issue by injecting v8::Isolate into JsEngine, that in general looks expected for testing purposes, though, it's rather overengineering here because we are not mocking v8::Isolate. > > 3) Allocation of 'v8::Isolate' should happen in 'class V8Isolate'. In the > current patch set of the present review, the only way that this class is > instantiated is by a call to 'make_shared'. There's no need to repeat the > allocation everywhere it's used. That allocation can go in the constructor of > 'V8Isolate' It's not clear what you mean. Allocation already happens in ScopedV8Isolate using `v8::Isolate::New()`. For better testing each test should be isolated as much as possible, so we should allocate what ever we need in each test. There is shared pointer to ScopedV8Isolate only to figure out the issue with memory usage. I would like to work on it after updating of v8 because it's seems more important now. > > 4) The most recent patch set makes this class non-copyable. 'unique_ptr' is > already non-copyable. Changing the isolate member from a raw pointer to > 'unique_ptr' will be just fine. Explained above, I don't see a reason to spread allocation and deallocation around, having it in ctr and dtr looks quite clean. > > 5) 'V8Isolate::GetIsolate()' should call 'get()' on its underlying pointer; this > is for clarity. And the name of this function can be just 'Get()' or even a > conversion operator to the underlying pointer type. I don't think we need to store v8::Isolate in std::unique_ptr here, it's overengineering, I have renamed the member method `GetIsolate` to `Get`. https://codereview.adblockplus.org/6233220328718336/diff/29334316/include/Adb... include/AdblockPlus/JsEngine.h:100: static JsEnginePtr New(const AppInfo& appInfo = AppInfo(), const ScopedV8IsolatePtr& isolate = ScopedV8IsolatePtr()); On 2016/01/26 14:48:57, Eric wrote: > Having an argument for the isolate in the constructor seems like a Bad Idea. > 'JsEngine' owns the isolate; it should be the one that creates it. It sounds like "having a clean testable architecture seems like a Bad Idea". As I told above, here it's indeed overengineering because we don't mock v8::Isolate, but we need it to figure out the current issue. > > It seems that the only reason to have an argument here is to support a shared > isolate for the unit tests. I'm not even sure sharing isolates is a good idea, > but if you need sharing for unit test, why not share a single instance of > 'JsEngine'? Nobody is saying that it's a good idea, I told in the description that it's a hack with which we have to live so far. Sharing of JsEngine is even worse idea because in that case we share even bigger state. https://codereview.adblockplus.org/6233220328718336/diff/29334316/include/Adb... include/AdblockPlus/JsEngine.h:246: v8::Isolate* GetIsolate() On 2016/01/26 14:48:58, Eric wrote: > Note: returns a raw pointer, not a smart one, even though its member is a smart > pointer. Better to call get() explicitly in such a case. By fact we need raw v8::Isolate for internal purposes and we don't need any wrapper around it. As it's discussed above, currently we have to have a member JsEngine::isolate of a shared type. I would certainly like to change it in future and we won't need to change places which work with JsEngine::GetIsolate. https://codereview.adblockplus.org/6233220328718336/diff/29334316/src/JsEngin... File src/JsEngine.cpp (right): https://codereview.adblockplus.org/6233220328718336/diff/29334316/src/JsEngin... src/JsEngine.cpp:79: : isolate(isolate ? isolate : std::make_shared<ScopedV8Isolate>()) On 2016/01/26 14:48:58, Eric wrote: > The hidden default in this initializer is both unnecessary and a bad idea. This > constructor is only being called twice. > 1) In 'CreateJsEngine()', where 'make_shared' is already being called. > 2) in 'JsEngine()' right below, which relies on this default. > Just pass the argument through. > > But, as I've said elsewhere, having an argument for the isolate here is a bad > idea in the first place. Done. https://codereview.adblockplus.org/6233220328718336/diff/29334316/test/BaseJs... File test/BaseJsTest.cpp (right): https://codereview.adblockplus.org/6233220328718336/diff/29334316/test/BaseJs... test/BaseJsTest.cpp:22: static AdblockPlus::ScopedV8IsolatePtr isolate = std::make_shared<AdblockPlus::ScopedV8Isolate>(); On 2016/01/26 14:48:58, Eric wrote: > What's the motivation for using a single isolate for the tests? I can imagine > that it's performance of the unit tests, but it doesn't say here. > > It _does_, however, make a difference to the semantics of the tests. By using a > single isolate for all tests, it makes the tests interact with each other > through potential side effects that a test leaves on the engine. That doesn't > seem exactly desirable. If we're testing side effects, we should be doing so > explicitly rather than implicitly through some artifact of the test harness. > > If performance is indeed the reason, and if it trumps isolation of the tests, > then that trade-off should be documented here. In other words, some code change > is needed here, even if it's only a comment. Have you read the description of the codereview? https://codereview.adblockplus.org/6233220328718336/diff/29334316/test/BaseJs... File test/BaseJsTest.h (right): https://codereview.adblockplus.org/6233220328718336/diff/29334316/test/BaseJs... test/BaseJsTest.h:141: AdblockPlus::JsEnginePtr createJsEngine(const AdblockPlus::AppInfo& appInfo = AdblockPlus::AppInfo()); On 2016/01/26 14:48:59, Eric wrote: > In a suggestion that also appears elsewhere in a different way, the 'appInfo' > argument could be omitted in favor of using application info that identifies the > application as "unit tests". How can it be omitted? In some tests we need it in some the default constructed AppInfo is used and there is no reason to create the second method that calls JsEngine::New with default AppInfo. https://codereview.adblockplus.org/6233220328718336/diff/29334316/test/BaseJs... test/BaseJsTest.h:141: AdblockPlus::JsEnginePtr createJsEngine(const AdblockPlus::AppInfo& appInfo = AdblockPlus::AppInfo()); On 2016/01/26 14:48:59, Eric wrote: > According to our style guide, this should be "CreateJsEngine". Done.
On 2016/01/27 15:06:11, sergei wrote: > Also I > would like to remind that we are not using branches, how it's done in a git > world, thus we don't group commits in a branch, each commit represents > desirabely small amount of logically independent changes. As well as, JIC, it > should not break anything. The point is important enough to get its own comment. Why aren't we using branches? That seems like a BAD IDEA. We should be using branches when we update (at least in general). We have a library with breaking API changes. When you have the dominant dependency changing its API, you just can't expect all your code will work across that boundary. The present change is a perfect case in point. Old v8 uses 'GetCurrent()'; new v8 uses 'New()'. They have different semantics. We can't really expect the same code to work correctly in both version. The fact that you have to alter the unit tests _at all_ to get the code to work is a good indication that using 'New()' when the current code expects 'GetCurrent()' is not a good idea. This is a larger issue than just this review, but I'm commenting here for initial continuity. We can move this discussion elsewhere.
On 2016/01/26 14:49:00, Eric wrote: > I would not commit this code with the current version of v8, as it does not > preserve current behavior. On 2016/01/27 15:06:11, sergei wrote: > How would you commit it then? I would commit it on a branch. The branch would be labelled something like "v8-4.3.15" to be really clear about v8 API dependencies. > I would like to remind that it's a separate commit > because it is a logically grouped refactoring changes This is not a refactoring change set. If you change behavior, it's not refactoring. I'll quote the authority on the matter, Martin Fowler: Taken from http://martinfowler.com/books/refactoring.html > Its essence is applying a series of small behavior-preserving transformations, each of which "too small to be worth doing". Note the phrase "behavior-preserving". When we switch from 'GetCurrent()' to 'New()', we are not preserving behavior. See also http://martinfowler.com/bliki/RefactoringMalapropism.html, in which Fowler speaks of how the word "refactoring" is used incorrectly.
I'm uncomfortable with this change as is. The fact that the destructor is not actually releasing resources is a big problem in my book. Proper resources don't act that way. I have a suspicion that the leak (if that's what it is) may be elsewhere in libadblockplus, but that's only a hunch. I can think of a few thing to try to track this down: 1) A test that instantiates many isolates using New()/Dispose() but that doesn't call anything with them. 2) A test that instantiates many 'JsEngine' objects. If test (1) passes and test (2) does not, then we have a leak in one of the other members. 3) There are _three_ relevant version of v8 around this issue, not just two: (a) the current version in our repository, (b) 4.3.15, and (c) the current one. It conceivable that this is a defect in 4.3.15 that has been fixed in a subsequent release. If test (1) fails on the current v8 release, we should probably work around it. And file a defect report. https://codereview.adblockplus.org/6233220328718336/diff/29334316/include/Adb... File include/AdblockPlus/JsEngine.h (right): https://codereview.adblockplus.org/6233220328718336/diff/29334316/include/Adb... include/AdblockPlus/JsEngine.h:53: * constructing and disposes it on destructing. On 2016/01/27 15:06:07, sergei wrote: > It seems your terminology is slightly different from the popular one. "Resource wrapper" is more common. https://codereview.adblockplus.org/6233220328718336/diff/29334316/include/Adb... include/AdblockPlus/JsEngine.h:59: ~ScopedV8Isolate(); On 2016/01/27 15:06:09, sergei wrote: > It's also undesirable to include third party library headers > in our headers That's reasonable. https://codereview.adblockplus.org/6233220328718336/diff/29334316/include/Adb... include/AdblockPlus/JsEngine.h:74: typedef std::shared_ptr<ScopedV8Isolate> ScopedV8IsolatePtr; On 2016/01/27 15:06:08, sergei wrote: > Now it's used in several places in JsEngine and once in BaseJsTest.cpp, it's > convenient to have a typedef for it. If it were used only a couple of times, > there certainly would not be typedef for it. It does not need to be used in any of those places. It can be used solely inside 'V8Isolate'. > std::unique_ptr with custom deleter You don't need a custom deleter. Apparently I was not clear enough. Declare the member variable thus: std::unique_ptr<std::v8::Isolate> isolate; Replace the 'make_shared' that are made _outside_ of the class with a call to 'new' _inside_ the constructor of the class. > I decided to create a special class whose responsibility is to > initialize and deinitialize `v8::Isolate`, so we have them located in one unit. Your motivation is fine, except that you only wrapped one of the two resources. One is the instance of 'v8::Isolate'. The other is the heap memory where that instance is located. Do the memory allocation inside the class also. > It's not clear what you mean. Allocation already happens in ScopedV8Isolate > using `v8::Isolate::New()`. There are two kinds of allocations here. The one you mention is the set of everything that 'v8::Isolate::New()' does. The second is a heap memory allocation for the instance of the isolate. > For better testing each test should be isolated as > much as possible, so we should allocate what ever we need in each test. There is > shared pointer to ScopedV8Isolate only to figure out the issue with memory > usage. I would like to work on it after updating of v8 because it's seems more > important now. That's a reasonable plan, but we need more here in order to execute it well. I don't particularly mind making a step backwards in the API for 'JsEngine', but if that's how you want to do it, you need (1) inline documentation (comments) identifying the regression and (2) a ticket to get it fixed in the future. On the other hand, I don't think you need to regress 'JsEngine' to get what you want. See comments for "BaseJsTest". > I don't think we need to store v8::Isolate in std::unique_ptr here, it's > overengineering It would be over-engineering to use both unique_ptr and shared_ptr. It is not over-engineering to replace shared_ptr with unique_ptr. https://codereview.adblockplus.org/6233220328718336/diff/29334316/include/Adb... include/AdblockPlus/JsEngine.h:100: static JsEnginePtr New(const AppInfo& appInfo = AppInfo(), const ScopedV8IsolatePtr& isolate = ScopedV8IsolatePtr()); On 2016/01/27 15:06:07, sergei wrote: > It sounds like "having a clean testable architecture seems like a Bad Idea". It's not clean, in my opinion. It's creating an unnecessary dependency. > Nobody is saying that it's a good idea, I told in the description that it's a > hack with which we have to live so far. Sharing of JsEngine is even worse idea > because in that case we share even bigger state. As long as you're going with a not-good idea, you might just as well go with one that you don't have to clean up after later. Sharing 'JsEngine' seems like a better idea if all you want is an interim solution. I have to say, though, that needing to do any of this smells bad. If the problem is that we're running out of memory when we allocate-and-then-deallocate what's supposed to be a resource multiple times, then it's not actually acting as a resource. I am growing more uncomfortable with unforeseen side effects of this change. https://codereview.adblockplus.org/6233220328718336/diff/29334316/test/BaseJs... File test/BaseJsTest.cpp (right): https://codereview.adblockplus.org/6233220328718336/diff/29334316/test/BaseJs... test/BaseJsTest.cpp:22: static AdblockPlus::ScopedV8IsolatePtr isolate = std::make_shared<AdblockPlus::ScopedV8Isolate>(); On 2016/01/27 15:06:10, sergei wrote: > Have you read the description of the codereview? Yes. It wasn't clear. It's only slightly more clear in the most recent patch set. https://codereview.adblockplus.org/6233220328718336/diff/29334316/test/BaseJs... File test/BaseJsTest.h (right): https://codereview.adblockplus.org/6233220328718336/diff/29334316/test/BaseJs... test/BaseJsTest.h:141: AdblockPlus::JsEnginePtr createJsEngine(const AdblockPlus::AppInfo& appInfo = AdblockPlus::AppInfo()); On 2016/01/27 15:06:10, sergei wrote: > How can it be omitted? In some tests we need it in some the default constructed > AppInfo is used and there is no reason to create the second method that calls > JsEngine::New with default AppInfo. OK. I'm not familiar with the unit test suite for this module. It does surprise me, however, that any tests rely on default 'AppInfo'. https://codereview.adblockplus.org/6233220328718336/diff/29334762/include/Adb... File include/AdblockPlus/JsEngine.h (right): https://codereview.adblockplus.org/6233220328718336/diff/29334762/include/Adb... include/AdblockPlus/JsEngine.h:256: ScopedV8IsolatePtr isolate; You should be able to declare this member 'const'. Doing that may cause some slight trouble with initialization, but getting to this declaration is a sign of a clean design. https://codereview.adblockplus.org/6233220328718336/diff/29334762/src/JsEngin... File src/JsEngine.cpp (right): https://codereview.adblockplus.org/6233220328718336/diff/29334762/src/JsEngin... src/JsEngine.cpp:69: { If you want code that works with the old version and new version, call "GetCurrent()" and test it against NULL. New versions of v8 did not eliminate the function; instead, it just always returns NULL. If "GetCurrent()" return non-null, use that isolate. If null, call "New()". http://izs.me/v8-docs/classv8_1_1Isolate.html https://codereview.adblockplus.org/6233220328718336/diff/29334762/src/JsEngin... src/JsEngine.cpp:86: JsEnginePtr result(new JsEngine(isolate ? isolate : std::make_shared<ScopedV8Isolate>())); It's the ternary expression that creates a hidden default in the initializer. If you want a default argument, use the standard C++ syntax for it. On the other hand, I don't think we should be allocate outside the class at all, but if you have to, at least do it right. https://codereview.adblockplus.org/6233220328718336/diff/29334762/src/JsValue... File src/JsValue.cpp (right): https://codereview.adblockplus.org/6233220328718336/diff/29334762/src/JsValue... src/JsValue.cpp:44: } This code showed up in the new patch set. Does it belong here? It seems like it came from a different change. https://codereview.adblockplus.org/6233220328718336/diff/29334762/test/BaseJs... File test/BaseJsTest.cpp (right): https://codereview.adblockplus.org/6233220328718336/diff/29334762/test/BaseJs... test/BaseJsTest.cpp:23: return AdblockPlus::JsEngine::New(appInfo, isolate); As has been discussed elsewhere, I'd create a single static 'JsEngine' and share that. It would promote better encapsulation in the 'JsEngine' constructor, eliminating the isolate argument.
Does this code even compile against the current public tip for ABP-IE? I haven't pulled a patch from here to try and compile, but 'v8::Isolate' does not appear to define either 'New()' or 'Dispose()', at least not in my local repository.
I have filed the issue #3593 that contains a lot of information from the current discussion. On 2016/01/27 15:32:38, Eric wrote: > On 2016/01/27 15:06:11, sergei wrote: > > Also I > > would like to remind that we are not using branches, how it's done in a git > > world, thus we don't group commits in a branch, each commit represents > > desirabely small amount of logically independent changes. As well as, JIC, it > > should not break anything. > > The point is important enough to get its own comment. > > Why aren't we using branches? That seems like a BAD IDEA. We should be using > branches when we update (at least in general). I agree, we should use branches, but it's not possible with the current approach in particular with reviewing and committing process. We neither review a pull request which consists from a set of commits nor perform a development in branches which are merged, instead we review each commit independently or commit several reviews as one squashed commit. I would not like to commit collapsed changes when we can commit them separately as here. I don't mind to push them all at a row but don't want make one huge commit. I don't think it makes sense to continue to discuss it here. There is a discussion thread (Proposal: Changing development workflow to use feature branches) in intraforum regarding using of branches. > > We have a library with breaking API changes. When you have the dominant > dependency changing its API, you just can't expect all your code will work > across that boundary. > > The present change is a perfect case in point. Old v8 uses 'GetCurrent()'; new > v8 uses 'New()'. They have different semantics. We can't really expect the same > code to work correctly in both version. The fact that you have to alter the unit > tests _at all_ to get the code to work is a good indication that using 'New()' > when the current code expects 'GetCurrent()' is not a good idea. I disagree here. Why do you think that the current code expects `GetCurrent()`? First point is that tests should not share any state, using `GetCurrent()` they do share the state, it's the mistake in the current approach, which should be fixed. The second point is that GetCurrent of the current version of v8 allocates v8::Isolate and we don't deallocate it, that's also a bug which should be fixed. We could switch from `GetCurrent()` to `New()` even without updating of v8. Of course we can firstly fix the architecture, then switch from `GetCurrent()` to `New()` without that hack and afterwards we will not have to have it as a part of updating of v8 but right now it seems more important to get it working with new v8. As I told in the previous comments, I want to change it but later. > > This is a larger issue than just this review, but I'm commenting here for > initial continuity. We can move this discussion elsewhere. On 2016/01/27 17:21:04, Eric wrote: > I'm uncomfortable with this change as is. The fact that the destructor is not > actually releasing resources is a big problem in my book. Proper resources don't > act that way. I have a suspicion that the leak (if that's what it is) may be > elsewhere in libadblockplus, but that's only a hunch. > > I can think of a few thing to try to track this down: > > 1) A test that instantiates many isolates using New()/Dispose() but that doesn't > call anything with them. > > 2) A test that instantiates many 'JsEngine' objects. If test (1) passes and test > (2) does not, then we have a leak in one of the other members. These are good candidates for real tests for memory leakages. > > 3) There are _three_ relevant version of v8 around this issue, not just two: (a) > the current version in our repository, (b) 4.3.15, and (c) the current one. It > conceivable that this is a defect in 4.3.15 that has been fixed in a subsequent > release. > > If test (1) fails on the current v8 release, we should probably work around it. > And file a defect report. I ensure you, the leak is in libadblockplus, the first one is a circular reference problem. JsEngine has member `JsValuePtr globalJsObject;` which has `JsEnginePtr jsEngine;` member. That seems the obvois one. The second one is that 'adblockplus' creates free running timer threads (with very long firering intervals), each thread is represented as an instance of a TimeoutThread class which holds `JsValuePtr function;` and `JsValueList functionArguments;` which hold `JsEnginePtr jsEngine;` that holds isolate. It's also results in some race conditions in tests. https://codereview.adblockplus.org/6233220328718336/diff/29334316/include/Adb... File include/AdblockPlus/JsEngine.h (right): https://codereview.adblockplus.org/6233220328718336/diff/29334316/include/Adb... include/AdblockPlus/JsEngine.h:74: typedef std::shared_ptr<ScopedV8Isolate> ScopedV8IsolatePtr; On 2016/01/27 17:21:01, Eric wrote: > On 2016/01/27 15:06:08, sergei wrote: > > Now it's used in several places in JsEngine and once in BaseJsTest.cpp, it's > > convenient to have a typedef for it. If it were used only a couple of times, > > there certainly would not be typedef for it. > > It does not need to be used in any of those places. It can be used solely inside > 'V8Isolate'. Sorry, what can be used? Are you saying that std::shared_ptr<ScopedV8Isolate> can be used inside std::shared_ptr<ScopedV8Isolate>? > > > std::unique_ptr with custom deleter > > You don't need a custom deleter. Apparently I was not clear enough. Declare the > member variable thus: > > std::unique_ptr<std::v8::Isolate> isolate; > There is no `std::v8`, I guess `std::` is accidentally added here. How does `std::unique_ptr<v8::Isolate>` know that it should call v8::Isolate::Dispose() on the stored pointer without custom deleter? > Replace the 'make_shared' that are made _outside_ of the class with a call to > 'new' _inside_ the constructor of the class. It's not clear what you suggest to construct with `new` instead of `std::make_shared`, what particular "the class" is and, consequently, how it helps. > > > I decided to create a special class whose responsibility is to > > initialize and deinitialize `v8::Isolate`, so we have them located in one > unit. > > Your motivation is fine, except that you only wrapped one of the two resources. > One is the instance of 'v8::Isolate'. The other is the heap memory where that > instance is located. Do the memory allocation inside the class also. > > > It's not clear what you mean. Allocation already happens in ScopedV8Isolate > > using `v8::Isolate::New()`. > > There are two kinds of allocations here. The one you mention is the set of > everything that 'v8::Isolate::New()' does. The second is a heap memory > allocation for the instance of the isolate. Sorry, which heap allocation are you talking about? Heap memory allocation for `v8::Isolate` is performed by `v8::Isolate::New()`. > > > For better testing each test should be isolated as > > much as possible, so we should allocate what ever we need in each test. There > is > > shared pointer to ScopedV8Isolate only to figure out the issue with memory > > usage. I would like to work on it after updating of v8 because it's seems more > > important now. > > That's a reasonable plan, but we need more here in order to execute it well. I > don't particularly mind making a step backwards in the API for 'JsEngine', but > if that's how you want to do it, you need (1) inline documentation (comments) > identifying the regression and (2) a ticket to get it fixed in the future. It's added in the issue. I don't think we need to comment it everywhere in the code, however, I have added a comment regarding this parameter to the function documentation. > > On the other hand, I don't think you need to regress 'JsEngine' to get what you > want. See comments for "BaseJsTest". Answered in BaseJsTest. > > > I don't think we need to store v8::Isolate in std::unique_ptr here, it's > > overengineering > > It would be over-engineering to use both unique_ptr and shared_ptr. It is not > over-engineering to replace shared_ptr with unique_ptr. > It's not clear about what places you talking here already. https://codereview.adblockplus.org/6233220328718336/diff/29334316/include/Adb... include/AdblockPlus/JsEngine.h:100: static JsEnginePtr New(const AppInfo& appInfo = AppInfo(), const ScopedV8IsolatePtr& isolate = ScopedV8IsolatePtr()); On 2016/01/27 17:21:01, Eric wrote: > On 2016/01/27 15:06:07, sergei wrote: > > It sounds like "having a clean testable architecture seems like a Bad Idea". > > It's not clean, in my opinion. It's creating an unnecessary dependency. > > > Nobody is saying that it's a good idea, I told in the description that it's a > > hack with which we have to live so far. Sharing of JsEngine is even worse idea > > because in that case we share even bigger state. > > As long as you're going with a not-good idea, you might just as well go with one > that you don't have to clean up after later. Sharing 'JsEngine' seems like a > better idea if all you want is an interim solution. > > I have to say, though, that needing to do any of this smells bad. If the problem > is that we're running out of memory when we allocate-and-then-deallocate what's > supposed to be a resource multiple times, then it's not actually acting as a > resource. I am growing more uncomfortable with unforeseen side effects of this > change. > Answered in BaseJsTest (https://codereview.adblockplus.org/6233220328718336/diff/29334762/test/BaseJs...). https://codereview.adblockplus.org/6233220328718336/diff/29334762/include/Adb... File include/AdblockPlus/JsEngine.h (right): https://codereview.adblockplus.org/6233220328718336/diff/29334762/include/Adb... include/AdblockPlus/JsEngine.h:256: ScopedV8IsolatePtr isolate; On 2016/01/27 17:21:02, Eric wrote: > You should be able to declare this member 'const'. Doing that may cause some > slight trouble with initialization, but getting to this declaration is a sign of > a clean design. I don't think that it would be better here. I would like to have a transparent const-correctness. `const` before smart pointer type is rather error-prone because it can be accidentally wrongly interpreted as a const pointer, which is wrong, we still can call non-const methods of this pointer. In addition I think that a const method returning non-const pointer or reference is rather dangerous, of course there are exception, but here it's not that case. https://codereview.adblockplus.org/6233220328718336/diff/29334762/src/JsEngin... File src/JsEngine.cpp (right): https://codereview.adblockplus.org/6233220328718336/diff/29334762/src/JsEngin... src/JsEngine.cpp:69: { On 2016/01/27 17:21:03, Eric wrote: > If you want code that works with the old version and new version, call > "GetCurrent()" and test it against NULL. New versions of v8 did not eliminate > the function; instead, it just always returns NULL. If "GetCurrent()" return > non-null, use that isolate. If null, call "New()". > > http://izs.me/v8-docs/classv8_1_1Isolate.html It does work with new v8 because using `New` we run out of memory. https://codereview.adblockplus.org/6233220328718336/diff/29334762/src/JsEngin... src/JsEngine.cpp:86: JsEnginePtr result(new JsEngine(isolate ? isolate : std::make_shared<ScopedV8Isolate>())); On 2016/01/27 17:21:02, Eric wrote: > It's the ternary expression that creates a hidden default in the initializer. If > you want a default argument, use the standard C++ syntax for it. > > On the other hand, I don't think we should be allocate outside the class at all, > but if you have to, at least do it right. Done. https://codereview.adblockplus.org/6233220328718336/diff/29334762/src/JsValue... File src/JsValue.cpp (right): https://codereview.adblockplus.org/6233220328718336/diff/29334762/src/JsValue... src/JsValue.cpp:44: } On 2016/01/27 17:21:03, Eric wrote: > This code showed up in the new patch set. Does it belong here? It seems like it > came from a different change. No, and it's not introduced in this patch set. It's here because this change is based on another codereview. I have also noticed it yesterday and added a note about it into the description. https://codereview.adblockplus.org/6233220328718336/diff/29334762/test/BaseJs... File test/BaseJsTest.cpp (right): https://codereview.adblockplus.org/6233220328718336/diff/29334762/test/BaseJs... test/BaseJsTest.cpp:23: return AdblockPlus::JsEngine::New(appInfo, isolate); On 2016/01/27 17:21:03, Eric wrote: > As has been discussed elsewhere, I'd create a single static 'JsEngine' and share > that. It would promote better encapsulation in the 'JsEngine' constructor, > eliminating the isolate argument. We cannot share JsEngine because it is specifically initialized in most tests and we don't need an instance of JsEngine partially initialized in one test to be used in another one. For example, we set MockFileSystem as a "file system engine" and we don't want to be affected by that particular implementation and by its state in other tests.
On 2016/01/27 17:38:21, Eric wrote: > Does this code even compile against the current public tip for ABP-IE? I haven't > pulled a patch from here to try and compile, but 'v8::Isolate' does not appear > to define either 'New()' or 'Dispose()', at least not in my local repository. https://hg.adblockplus.org/libadblockplus/file/tip/dependencies https://hg.adblockplus.org/v8-googlesource/file/28969bee9861/include/v8.h#l4014 Generally speaking, it does work, however on the current tip of libadblockplus and there are some merging conflicts if you try to apply it without firstly applied https://codereview.adblockplus.org/5598762307158016/. If someone wants to try it out, one can obtain it from https://github.com/abby-sergz/libadblockplus/tree/msvs2013-incomplete. Pay attention, the branch is "msvs2013-incomplete" and don't forget to unload "ensure_dependencies" project because it requires to have working git in the environment.
On 2016/01/27 17:38:21, Eric wrote: > Does this code even compile against the current public tip for ABP-IE? I On 2016/01/28 14:47:20, sergei wrote: > https://hg.adblockplus.org/libadblockplus/file/tip/dependencies > https://hg.adblockplus.org/v8-googlesource/file/28969bee9861/include/v8.h#l4014 OK. I did _not_ ask about other repositories, I asked about ABP-IE with repository named "adblockplusie". This question isn't about v8 in general, but about the specific version of v8 that the current tip of ABP-IE relies upon. I'll just take your actual answer as "no, it doesn't compile with current ABP-IE". Since this change set is still marked with #1197, which references Visual Studio, which means it still is actually about ABP-IE in a fundamental way, I strongly suggest that it apply against the current tip for ABP-IE. So either unmark it as about #1197 or update ABP-IE to use a version that it can.
On 2016/01/28 14:02:52, sergei wrote: > I have filed the issue #3593 that contains a lot of information from the current > discussion. I read it. It's a much better description of what's going on than simply referencing #1197. I suggest moving the stack trace out of the current issue description and into a comment on #3593. > I agree, we should use branches, but it's not possible with the current approach > in particular with reviewing and committing process. [...] > There is a discussion thread (Proposal: Changing > development workflow to use feature branches) in intraforum regarding using of > branches. That thread is about using lightweight, anonymous branches. It's not the same situation as what we need to deal with v8 version upgrades, which don't act the same way. We have a major dependency here with changing API that breaks old code. Using a regular named branch would be just fine here. Procedure would be pretty simple. The first commit to such a branch would be dependency to some newer version of v8. Everything on the branch is changes to make the new version work. When everything works, merge it back to the main branch. With a feature branch, you don't want to break the build. When you have a dependency with a breaking API change, you can't help but break the build. The only change to the review process would be to make sure any change set not intended for the main branch name what branch it's destined for. There's no field in the description form for that, which would be convenient, but it's certainly not necessary. > I disagree here. Why do you think that the current code expects `GetCurrent()`? Because the copy of v8 that's in my repository for ABP-IE uses 'GetCurrent()'. Now I think we've determined that the copy of v8 in ABP-IE is not the one you're working with here. > First point is that tests should not share any state, using `GetCurrent()` they > do share the state, it's the mistake in the current approach, which should be > fixed. If this problem doesn't have its own ticket, it should get one. The lack of documentation on this specific point was the origin of a fair bit of confusion. > I ensure you, the leak is in libadblockplus, the first one is a circular > reference problem. JsEngine has member `JsValuePtr globalJsObject;` which has > `JsEnginePtr jsEngine;` member. Does this have its own ticket? Since the member variable 'JsValue::jsEngine' is a 'shared_ptr', it would seem that the destructor for 'JsEngine' instances is never getting called if there any stray 'JsValue' instances still in existence, and that would mean that 'Dispose()' is never getting called. Changing this from 'shared_ptr' to 'weak_ptr' should fix it, but it means checking for 'nullptr' everywhere. > The second one is > that 'adblockplus' creates free running timer threads [...] Does this have its own ticket?
https://codereview.adblockplus.org/6233220328718336/diff/29334316/include/Adb... File include/AdblockPlus/JsEngine.h (right): https://codereview.adblockplus.org/6233220328718336/diff/29334316/include/Adb... include/AdblockPlus/JsEngine.h:74: typedef std::shared_ptr<ScopedV8Isolate> ScopedV8IsolatePtr; On 2016/01/28 14:02:50, sergei wrote: > Sorry, what can be used? Are you saying that std::shared_ptr<ScopedV8Isolate> > can be used inside std::shared_ptr<ScopedV8Isolate>? Of course not. I'm going to post a review. It will be quicker that trying to explain it yet again. > I have added a comment regarding this parameter to the function > documentation. That will work. As long as we have documentation a regression that necessary as a workaround, we're OK. https://codereview.adblockplus.org/6233220328718336/diff/29334762/include/Adb... File include/AdblockPlus/JsEngine.h (right): https://codereview.adblockplus.org/6233220328718336/diff/29334762/include/Adb... include/AdblockPlus/JsEngine.h:256: ScopedV8IsolatePtr isolate; On 2016/01/28 14:02:50, sergei wrote: > I don't think that it would be better here. I would like to have a transparent > const-correctness. `const` before smart pointer type is rather error-prone I've said before that this member should _not_ be a smart pointer. The smart pointer should be inside another class. https://codereview.adblockplus.org/6233220328718336/diff/29334762/test/BaseJs... File test/BaseJsTest.cpp (right): https://codereview.adblockplus.org/6233220328718336/diff/29334762/test/BaseJs... test/BaseJsTest.cpp:23: return AdblockPlus::JsEngine::New(appInfo, isolate); On 2016/01/28 14:02:51, sergei wrote: > On 2016/01/27 17:21:03, Eric wrote: > > As has been discussed elsewhere, I'd create a single static 'JsEngine' and > share > > that. It would promote better encapsulation in the 'JsEngine' constructor, > > eliminating the isolate argument. > > We cannot share JsEngine because it is specifically initialized in most tests > and we don't need an instance of JsEngine partially initialized in one test to > be used in another one. For example, we set MockFileSystem as a "file system > engine" and we don't want to be affected by that particular implementation and > by its state in other tests. Acknowledged. https://codereview.adblockplus.org/6233220328718336/diff/29334865/include/Adb... File include/AdblockPlus/JsEngine.h (right): https://codereview.adblockplus.org/6233220328718336/diff/29334865/include/Adb... include/AdblockPlus/JsEngine.h:99: * as a temporay hack for tests, it will go away. Issue #3593. spelling "temporary"
On 2016/01/28 15:31:21, Eric wrote: > On 2016/01/27 17:38:21, Eric wrote: > > Does this code even compile against the current public tip for ABP-IE? I > > On 2016/01/28 14:47:20, sergei wrote: > > https://hg.adblockplus.org/libadblockplus/file/tip/dependencies > > > https://hg.adblockplus.org/v8-googlesource/file/28969bee9861/include/v8.h#l4014 > > OK. I did _not_ ask about other repositories, I asked about ABP-IE with > repository named "adblockplusie". This question isn't about v8 in general, but > about the specific version of v8 that the current tip of ABP-IE relies upon. > > I'll just take your actual answer as "no, it doesn't compile with current > ABP-IE". Where did I say "no it doesn't compile with current ABP-IE"? Could you post what is on the line 4014 in your libadblockplus/third_party/v8/include/v8.h? It should be https://hg.adblockplus.org/v8/file/062a08f5e129/include/v8.h#l4014. > > Since this change set is still marked with #1197, which references Visual > Studio, which means it still is actually about ABP-IE in a fundamental way, I > strongly suggest that it apply against the current tip for ABP-IE. So either > unmark it as about #1197 or update ABP-IE to use a version that it can. The title of the issue #1197 is "change local copy of v8 (to 4.3.15) to work with Visual Studio 2013", the module is "Libadblockplus", the commit is indeed a part of #1197, the changes are for libadblockplus. Where is ABP-IE?
On 2016/01/28 16:18:35, Eric wrote: > ... > I suggest moving the stack trace out of the current issue description and into a > comment on #3593. Done. > > I agree, we should use branches, but it's not possible with the current > approach > > in particular with reviewing and committing process. > [...] > > There is a discussion thread (Proposal: Changing > > development workflow to use feature branches) in intraforum regarding using of > > branches. > > That thread is about using lightweight, anonymous branches. It's not the same > situation as what we need to deal with v8 version upgrades, which don't act the > same way. We have a major dependency here with changing API that breaks old > code. Using a regular named branch would be just fine here. Procedure would be > pretty simple. The first commit to such a branch would be dependency to some > newer version of v8. Everything on the branch is changes to make the new version > work. When everything works, merge it back to the main branch. With a feature > branch, you don't want to break the build. When you have a dependency with a > breaking API change, you can't help but break the build. > > The only change to the review process would be to make sure any change set not > intended for the main branch name what branch it's destined for. There's no > field in the description form for that, which would be convenient, but it's > certainly not necessary. As I told, we are not working with pull requests and we are using mercurial, which already tries to make the life very complicated, our review process can be very slow and maintaining of a pushed branch with mercurial becames very painful. This discussion is pointless here. Of course I see the reason to use branches but in that particular case there is no reason to put it into another branch. > > > I disagree here. Why do you think that the current code expects > `GetCurrent()`? > > Because the copy of v8 that's in my repository for ABP-IE uses 'GetCurrent()'. > Now I think we've determined that the copy of v8 in ABP-IE is not the one you're > working with here. Answered in the previous message. > > First point is that tests should not share any state, using `GetCurrent()` > they > > do share the state, it's the mistake in the current approach, which should be > > fixed. > > If this problem doesn't have its own ticket, it should get one. The lack of > documentation on this specific point was the origin of a fair bit of confusion. > > > I ensure you, the leak is in libadblockplus, the first one is a circular > > reference problem. JsEngine has member `JsValuePtr globalJsObject;` which has > > `JsEnginePtr jsEngine;` member. > > Does this have its own ticket? > > Since the member variable 'JsValue::jsEngine' is a 'shared_ptr', it would seem > that the destructor for 'JsEngine' instances is never getting called if there > any stray 'JsValue' instances still in existence, and that would mean that > 'Dispose()' is never getting called. Changing this from 'shared_ptr' to > 'weak_ptr' should fix it, but it means checking for 'nullptr' everywhere. > > > The second one is > > that 'adblockplus' creates free running timer threads [...] > > Does this have its own ticket? I have created tickets, #3594 and #3595.
https://codereview.adblockplus.org/6233220328718336/diff/29334762/include/Adb... File include/AdblockPlus/JsEngine.h (right): https://codereview.adblockplus.org/6233220328718336/diff/29334762/include/Adb... include/AdblockPlus/JsEngine.h:256: ScopedV8IsolatePtr isolate; On 2016/01/28 16:42:41, Eric wrote: > On 2016/01/28 14:02:50, sergei wrote: > > I don't think that it would be better here. I would like to have a transparent > > const-correctness. `const` before smart pointer type is rather error-prone > > I've said before that this member should _not_ be a smart pointer. The smart > pointer should be inside another class. I have just explained my point of view regarding using `const` before smart pointers. What concerns the smart pointer, I don't think we need any of them here. It's discussed here https://codereview.adblockplus.org/6233220328718336/diff/29334316/include/Adb... On 2016/01/27 15:06:08, sergei wrote: > ... I decided to create a special class whose responsibility is to > initialize and deinitialize `v8::Isolate`, so we have them located in one unit. > Having that I would like to have rather member ScopedV8Isolate than > std::unique_ptr. https://codereview.adblockplus.org/6233220328718336/diff/29334865/include/Adb... File include/AdblockPlus/JsEngine.h (right): https://codereview.adblockplus.org/6233220328718336/diff/29334865/include/Adb... include/AdblockPlus/JsEngine.h:99: * as a temporay hack for tests, it will go away. Issue #3593. On 2016/01/28 16:42:42, Eric wrote: > spelling "temporary" Done.
On 2016/01/29 10:29:30, sergei wrote: > Where did I say "no it doesn't compile with current ABP-IE"? When you avoided answering a simple yes-no question. I just verified it doesn't compile. I downloaded the patch set and it doesn't even apply against the current libadblock version referenced in ABP-IE. So what version is this change set supposed to be applied against?
https://codereview.adblockplus.org/6233220328718336/diff/29334762/include/Adb... File include/AdblockPlus/JsEngine.h (right): https://codereview.adblockplus.org/6233220328718336/diff/29334762/include/Adb... include/AdblockPlus/JsEngine.h:256: ScopedV8IsolatePtr isolate; On 2016/01/29 11:31:14, sergei wrote: > I have just explained my point of view regarding using `const` before smart > pointers. I am seriously beginning to think you don't actually read what I say with any degree of care, because you're arguing against a straw man here. That's _not_ what I was suggesting.
LGTM
On 2016/05/02 16:04:33, Oleksandr wrote: > LGTM I have tried to follow the discussion here, I don't really like the suggested implementation too, but it does fix the leak and brings us closer to VS2013. Unfortunately I did not follow Eric's suggestion to move the smart pointer ScopedV8IsolatePtr to another class (I assume it is related to the discussion here: https://codereview.adblockplus.org/6233220328718336/diff/29334316/include/Adb..., so there should be a separate code-review for that). The current code is fine by me either way. I did not verify this actually compiles/works, hence it Looks-Good-To-Me no Works-For-Me.
rebased + a small fix. I would like to ask you to check it again just in case. I'm going to commit it. It compiles for android and all tests pass on linux and windows.
https://codereview.adblockplus.org/6233220328718336/diff/29335080/src/JsEngin... File src/JsEngine.cpp (right): https://codereview.adblockplus.org/6233220328718336/diff/29335080/src/JsEngin... src/JsEngine.cpp:85: V8Initializer::Init(); Is there a reason for this move? I think it was better when Init() was called here. We assume the caller uses ScopedV8Isolate, but what if that's not the case?
https://codereview.adblockplus.org/6233220328718336/diff/29335080/src/JsEngin... File src/JsEngine.cpp (right): https://codereview.adblockplus.org/6233220328718336/diff/29335080/src/JsEngin... src/JsEngine.cpp:85: V8Initializer::Init(); On 2016/05/23 10:32:55, Oleksandr wrote: > Is there a reason for this move? I think it was better when Init() was called > here. We assume the caller uses ScopedV8Isolate, but what if that's not the > case? Yes, there is a reason. Before we call `v8::Isolate::New()` (it's called in the ctr of `ScopedV8Isolate`) we need to call `V8Initializer::Init();`. So the caller should call `V8Initializer::Init()` before constructing isolate, but the caller is likely not aware about it, especially because `isolate` is a default value parameter which should not bother the caller with additional call of `V8Initializer::Init()`.
LGTM 
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
