Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(54)

Issue 6233220328718336: Issue #3593, #1197- fix isolate management (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
4 years, 5 months ago by sergei
Modified:
3 years, 5 months ago
Reviewers:
Oleksandr
CC:
Felix Dahlke, Eric, René Jeschke
Visibility:
Public.

Description

Please 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 #

Unified diffs Side-by-side diffs Delta from patch set Stats (+108 lines, -31 lines) Patch
M include/AdblockPlus/JsEngine.h View 1 2 3 4 5 6 7 3 chunks +41 lines, -3 lines 0 comments Download
M libadblockplus.gyp View 1 2 3 4 5 6 7 8 1 chunk +2 lines, -0 lines 0 comments Download
M src/JsContext.cpp View 1 2 1 chunk +3 lines, -3 lines 0 comments Download
M src/JsEngine.cpp View 1 2 3 4 5 6 7 8 4 chunks +25 lines, -14 lines 0 comments Download
M src/JsValue.cpp View 1 2 3 4 5 6 7 8 4 chunks +7 lines, -7 lines 0 comments Download
M test/BaseJsTest.h View 1 2 3 4 2 chunks +3 lines, -1 line 0 comments Download
A test/BaseJsTest.cpp View 1 2 3 4 5 6 1 chunk +24 lines, -0 lines 0 comments Download
M test/FilterEngine.cpp View 1 2 3 4 5 6 7 8 1 chunk +1 line, -1 line 0 comments Download
M test/Prefs.cpp View 1 2 3 4 1 chunk +1 line, -1 line 0 comments Download
M test/UpdateCheck.cpp View 1 2 3 4 1 chunk +1 line, -1 line 0 comments Download

Messages

Total messages: 32
sergei
4 years, 5 months ago (2015-06-11 13:29:14 UTC) #1
Eric
Simplifying this change just to initialization order may well let this test pass. https://codereview.adblockplus.org/6233220328718336/diff/5629499534213120/include/AdblockPlus/JsEngine.h File ...
4 years, 3 months ago (2015-08-05 22:29:17 UTC) #2
sergei
https://codereview.adblockplus.org/6233220328718336/diff/5629499534213120/include/AdblockPlus/JsEngine.h File include/AdblockPlus/JsEngine.h (right): https://codereview.adblockplus.org/6233220328718336/diff/5629499534213120/include/AdblockPlus/JsEngine.h#newcode58 include/AdblockPlus/JsEngine.h:58: class ScopedV8Isolate On 2015/08/05 22:29:16, Eric wrote: > There's ...
4 years ago (2015-11-16 16:52:10 UTC) #3
Eric
https://codereview.adblockplus.org/6233220328718336/diff/5629499534213120/include/AdblockPlus/JsEngine.h File include/AdblockPlus/JsEngine.h (right): https://codereview.adblockplus.org/6233220328718336/diff/5629499534213120/include/AdblockPlus/JsEngine.h#newcode72 include/AdblockPlus/JsEngine.h:72: */ On 2015/11/16 16:52:09, sergei wrote: > I've inherited ...
4 years ago (2015-11-17 21:27:43 UTC) #4
Eric
I sent the previous batch of comments, which were all about previous comments, before I ...
4 years ago (2015-11-17 21:55:05 UTC) #5
sergei
On 2015/11/17 21:55:05, Eric wrote: > I sent the previous batch of comments, which were ...
3 years, 9 months ago (2016-01-22 10:41:04 UTC) #6
Eric
On 2015/11/17 21:55:05, Eric wrote: > Having seen the new patch set, it doesn't seem ...
3 years, 9 months ago (2016-01-25 13:50:38 UTC) #7
Eric
It is also necessary to know what version or versions of v8 this change set ...
3 years, 9 months ago (2016-01-25 14:13:01 UTC) #8
sergei
On 2016/01/25 14:13:01, Eric wrote: > It is also necessary to know what version or ...
3 years, 9 months ago (2016-01-25 15:33:36 UTC) #9
Eric
My comments assume that the present change set will only be applied against a new ...
3 years, 9 months ago (2016-01-26 14:49:00 UTC) #10
Eric
On 2016/01/25 14:13:01, Eric wrote: > In particular, I cannot distinguish between two situations. > ...
3 years, 9 months ago (2016-01-26 14:57:45 UTC) #11
sergei
On 2016/01/26 14:49:00, Eric wrote: > My comments assume that the present change set will ...
3 years, 9 months ago (2016-01-27 15:06:11 UTC) #12
Eric
On 2016/01/27 15:06:11, sergei wrote: > Also I > would like to remind that we ...
3 years, 9 months ago (2016-01-27 15:32:38 UTC) #13
Eric
On 2016/01/26 14:49:00, Eric wrote: > I would not commit this code with the current ...
3 years, 9 months ago (2016-01-27 15:48:45 UTC) #14
Eric
I'm uncomfortable with this change as is. The fact that the destructor is not actually ...
3 years, 9 months ago (2016-01-27 17:21:04 UTC) #15
Eric
Does this code even compile against the current public tip for ABP-IE? I haven't pulled ...
3 years, 9 months ago (2016-01-27 17:38:21 UTC) #16
sergei
I have filed the issue #3593 that contains a lot of information from the current ...
3 years, 9 months ago (2016-01-28 14:02:52 UTC) #17
sergei
On 2016/01/27 17:38:21, Eric wrote: > Does this code even compile against the current public ...
3 years, 9 months ago (2016-01-28 14:47:20 UTC) #18
Eric
On 2016/01/27 17:38:21, Eric wrote: > Does this code even compile against the current public ...
3 years, 9 months ago (2016-01-28 15:31:21 UTC) #19
Eric
On 2016/01/28 14:02:52, sergei wrote: > I have filed the issue #3593 that contains a ...
3 years, 9 months ago (2016-01-28 16:18:35 UTC) #20
Eric
https://codereview.adblockplus.org/6233220328718336/diff/29334316/include/AdblockPlus/JsEngine.h File include/AdblockPlus/JsEngine.h (right): https://codereview.adblockplus.org/6233220328718336/diff/29334316/include/AdblockPlus/JsEngine.h#newcode74 include/AdblockPlus/JsEngine.h:74: typedef std::shared_ptr<ScopedV8Isolate> ScopedV8IsolatePtr; On 2016/01/28 14:02:50, sergei wrote: > ...
3 years, 9 months ago (2016-01-28 16:42:43 UTC) #21
sergei
On 2016/01/28 15:31:21, Eric wrote: > On 2016/01/27 17:38:21, Eric wrote: > > Does this ...
3 years, 9 months ago (2016-01-29 10:29:30 UTC) #22
sergei
On 2016/01/28 16:18:35, Eric wrote: > ... > I suggest moving the stack trace out ...
3 years, 9 months ago (2016-01-29 11:11:09 UTC) #23
sergei
https://codereview.adblockplus.org/6233220328718336/diff/29334762/include/AdblockPlus/JsEngine.h File include/AdblockPlus/JsEngine.h (right): https://codereview.adblockplus.org/6233220328718336/diff/29334762/include/AdblockPlus/JsEngine.h#newcode256 include/AdblockPlus/JsEngine.h:256: ScopedV8IsolatePtr isolate; On 2016/01/28 16:42:41, Eric wrote: > On ...
3 years, 9 months ago (2016-01-29 11:31:16 UTC) #24
Eric
On 2016/01/29 10:29:30, sergei wrote: > Where did I say "no it doesn't compile with ...
3 years, 9 months ago (2016-02-09 14:53:38 UTC) #25
Eric
https://codereview.adblockplus.org/6233220328718336/diff/29334762/include/AdblockPlus/JsEngine.h File include/AdblockPlus/JsEngine.h (right): https://codereview.adblockplus.org/6233220328718336/diff/29334762/include/AdblockPlus/JsEngine.h#newcode256 include/AdblockPlus/JsEngine.h:256: ScopedV8IsolatePtr isolate; On 2016/01/29 11:31:14, sergei wrote: > I ...
3 years, 9 months ago (2016-02-09 14:59:14 UTC) #26
Oleksandr
LGTM
3 years, 6 months ago (2016-05-02 16:04:33 UTC) #27
Oleksandr
On 2016/05/02 16:04:33, Oleksandr wrote: > LGTM I have tried to follow the discussion here, ...
3 years, 6 months ago (2016-05-02 16:27:01 UTC) #28
sergei
rebased + a small fix. I would like to ask you to check it again ...
3 years, 5 months ago (2016-05-23 10:13:09 UTC) #29
Oleksandr
https://codereview.adblockplus.org/6233220328718336/diff/29335080/src/JsEngine.cpp File src/JsEngine.cpp (right): https://codereview.adblockplus.org/6233220328718336/diff/29335080/src/JsEngine.cpp#newcode85 src/JsEngine.cpp:85: V8Initializer::Init(); Is there a reason for this move? I ...
3 years, 5 months ago (2016-05-23 10:32:57 UTC) #30
sergei
https://codereview.adblockplus.org/6233220328718336/diff/29335080/src/JsEngine.cpp File src/JsEngine.cpp (right): https://codereview.adblockplus.org/6233220328718336/diff/29335080/src/JsEngine.cpp#newcode85 src/JsEngine.cpp:85: V8Initializer::Init(); On 2016/05/23 10:32:55, Oleksandr wrote: > Is there ...
3 years, 5 months ago (2016-05-23 10:39:13 UTC) #31
Oleksandr
3 years, 5 months ago (2016-05-23 10:43:07 UTC) #32
LGTM
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 87257f5