Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
220203005:
Oilpan: introduce sticky forcedForTesting flag to ensure that a precise (Closed)
|
Created:
6 years, 8 months ago by Mads Ager (chromium) Modified:
6 years, 8 months ago Reviewers:
Vyacheslav Egorov (Chromium), zerny-chromium, Erik Corry, haraken, oilpan-reviews, wibling-chromium, kouhei (in TOK) CC:
blink-reviews, shans, rjwright, alancutter (OOO until 2018), Mike Lawther (Google), kouhei+heap_chromium.org, dstockwell, Timothy Loh, darktears, Steve Block, dino_apple.com, Eric Willigers Base URL:
svn://svn.chromium.org/blink/trunk Visibility:
Public. |
DescriptionOilpan: introduce sticky forcedForTesting flag to ensure that a precise
GC is performed at the next return to the event loop.
Use this in svg animation tests to be able to reliably check that
objects die when expected.
R=erik.corry@gmail.com, haraken@chromium.org, kouhei@chromium.org, oilpan-reviews@chromium.org, vegorov@chromium.org
NOTRY=true
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=170648
Patch Set 1 #
Total comments: 20
Patch Set 2 : Address comments. #Patch Set 3 : Address comments. #
Total comments: 2
Patch Set 4 : Rename flag #Patch Set 5 : Fix js-test.js use in test. #
Messages
Total messages: 24 (0 generated)
lgtm https://codereview.chromium.org/220203005/diff/1/LayoutTests/fast/forms/assoc... File LayoutTests/fast/forms/associatedFormControls-leak-nodes.html (right): https://codereview.chromium.org/220203005/diff/1/LayoutTests/fast/forms/assoc... LayoutTests/fast/forms/associatedFormControls-leak-nodes.html:11: testRunner.waitUntilDone(); Can't we wait at the end to ensure we don't exit the test until done? https://codereview.chromium.org/220203005/diff/1/LayoutTests/resources/js-tes... File LayoutTests/resources/js-test.js (right): https://codereview.chromium.org/220203005/diff/1/LayoutTests/resources/js-tes... LayoutTests/resources/js-test.js:655: // can artificially keep objects alive. Therfore, test that need to check NIT: Therfore -> Therefore test -> tests https://codereview.chromium.org/220203005/diff/1/LayoutTests/resources/js-tes... LayoutTests/resources/js-test.js:659: GCController.collect(); Consider adding a comment explaining why we do several GC's and not just one. https://codereview.chromium.org/220203005/diff/1/LayoutTests/svg/animations/s... File LayoutTests/svg/animations/smil-leak-dynamically-added-element-instances.svg (right): https://codereview.chromium.org/220203005/diff/1/LayoutTests/svg/animations/s... LayoutTests/svg/animations/smil-leak-dynamically-added-element-instances.svg:38: // can artificially keep objects alive. Therfore, test that need to check Ditto. https://codereview.chromium.org/220203005/diff/1/LayoutTests/svg/animations/s... LayoutTests/svg/animations/smil-leak-dynamically-added-element-instances.svg:73: } Could you do a testRunner.waitUntilDone() here to avoid exiting until the gc's are complete? https://codereview.chromium.org/220203005/diff/1/LayoutTests/svg/animations/s... File LayoutTests/svg/animations/smil-leak-element-instances.svg (right): https://codereview.chromium.org/220203005/diff/1/LayoutTests/svg/animations/s... LayoutTests/svg/animations/smil-leak-element-instances.svg:38: // can artificially keep objects alive. Therfore, test that need to check Ditto. https://codereview.chromium.org/220203005/diff/1/LayoutTests/svg/animations/s... LayoutTests/svg/animations/smil-leak-element-instances.svg:74: } Also can't we waitUntilDone here to not exit until the gc is complete? https://codereview.chromium.org/220203005/diff/1/LayoutTests/svg/animations/s... File LayoutTests/svg/animations/smil-leak-elements.svg (right): https://codereview.chromium.org/220203005/diff/1/LayoutTests/svg/animations/s... LayoutTests/svg/animations/smil-leak-elements.svg:36: // can artificially keep objects alive. Therfore, test that need to check Ditto. https://codereview.chromium.org/220203005/diff/1/Source/heap/ThreadState.h File Source/heap/ThreadState.h (right): https://codereview.chromium.org/220203005/diff/1/Source/heap/ThreadState.h#ne... Source/heap/ThreadState.h:281: void performPendingGC(StackState); NIT: Could be made private. https://codereview.chromium.org/220203005/diff/1/Source/heap/ThreadState.h#ne... Source/heap/ThreadState.h:290: bool forcedForTesting(); NIT: Could be made private.
I'm not happy to introduce an asynchronous gc() mechanism into layout tests, but I'm getting to conclude that we cannot avoid it: - As long as we use conservative stack scanning, we need to trigger a GC at the end of an event loop to get deterministic results. - This means that we need to use an asynchronous gc() in layout tests. So, basically I agree with this CL. However, we might want to consider a couple of things to improve the situation: - In the oilpan world, nothing is guaranteed about a synchronous gc(). Garbage might be collected but might not be collected. Leaving the potential flakiness is not allowed in layout tests, so we'll need to completely deprecate synchronous gc()s when shipping oilpan. I don't think it's acceptable to take an approach where we rewrite tests incrementally every time we hit some flakiness. I'd propose to rewrite all tests in a reasonable timeline. - We don't want to duplicate collectGarbage() methods in a lot of tests. Shall we implement testRunner.gc(callback)? Then we can do the deprecation in the following steps: (1) Rename gc() to gcDeprecated(). This includes gc() in js-test.js and other gc()s written in a test file directly. (2) Replace all gcDeprecated() with testRunner.gc(callback). (3) Remove gcDeprecated() from layout tests. What do you think?
On 2014/04/01 13:37:27, haraken wrote: > I'm not happy to introduce an asynchronous gc() mechanism into layout tests, but > I'm getting to conclude that we cannot avoid it: > > - As long as we use conservative stack scanning, we need to trigger a GC at the > end of an event loop to get deterministic results. > - This means that we need to use an asynchronous gc() in layout tests. > > So, basically I agree with this CL. However, we might want to consider a couple > of things to improve the situation: > > - In the oilpan world, nothing is guaranteed about a synchronous gc(). Garbage > might be collected but might not be collected. Leaving the potential flakiness > is not allowed in layout tests, so we'll need to completely deprecate > synchronous gc()s when shipping oilpan. I don't think it's acceptable to take an > approach where we rewrite tests incrementally every time we hit some flakiness. > I'd propose to rewrite all tests in a reasonable timeline. > > - We don't want to duplicate collectGarbage() methods in a lot of tests. Shall > we implement testRunner.gc(callback)? Then we can do the deprecation in the > following steps: > > (1) Rename gc() to gcDeprecated(). This includes gc() in js-test.js and other > gc()s written in a test file directly. > (2) Replace all gcDeprecated() with testRunner.gc(callback). > (3) Remove gcDeprecated() from layout tests. > > What do you think? We definitely need to rewrite all of these to get deterministic results. However, we need to start somewhere. I have added this to js-test.js for now which is used by a lot of tests so it should cover most cases. As follow-up changes we should continue the replacement of gc() with collectGarbage(callback). I will keep that going as a background task.
https://codereview.chromium.org/220203005/diff/1/LayoutTests/fast/forms/assoc... File LayoutTests/fast/forms/associatedFormControls-leak-nodes.html (right): https://codereview.chromium.org/220203005/diff/1/LayoutTests/fast/forms/assoc... LayoutTests/fast/forms/associatedFormControls-leak-nodes.html:11: testRunner.waitUntilDone(); On 2014/04/01 13:26:09, wibling-chromium wrote: > Can't we wait at the end to ensure we don't exit the test until done? This is setting a flag in the test runner that this test is not done until it calls notifyDone and it is usually done in the beginning of the test. The only thing that matters is that it is called synchronously here. Otherwise the layout test will exit without going back to the event loop. So, it could be at the end of this straight-line code, but the usual placement is at the top so I'll leave it there. https://codereview.chromium.org/220203005/diff/1/LayoutTests/resources/js-tes... File LayoutTests/resources/js-test.js (right): https://codereview.chromium.org/220203005/diff/1/LayoutTests/resources/js-tes... LayoutTests/resources/js-test.js:655: // can artificially keep objects alive. Therfore, test that need to check On 2014/04/01 13:26:09, wibling-chromium wrote: > NIT: Therfore -> Therefore > test -> tests Done. https://codereview.chromium.org/220203005/diff/1/LayoutTests/resources/js-tes... LayoutTests/resources/js-test.js:659: GCController.collect(); On 2014/04/01 13:26:09, wibling-chromium wrote: > Consider adding a comment explaining why we do several GC's and not just one. Yes, done. https://codereview.chromium.org/220203005/diff/1/LayoutTests/svg/animations/s... File LayoutTests/svg/animations/smil-leak-dynamically-added-element-instances.svg (right): https://codereview.chromium.org/220203005/diff/1/LayoutTests/svg/animations/s... LayoutTests/svg/animations/smil-leak-dynamically-added-element-instances.svg:38: // can artificially keep objects alive. Therfore, test that need to check On 2014/04/01 13:26:09, wibling-chromium wrote: > Ditto. Done. https://codereview.chromium.org/220203005/diff/1/LayoutTests/svg/animations/s... LayoutTests/svg/animations/smil-leak-dynamically-added-element-instances.svg:73: } On 2014/04/01 13:26:09, wibling-chromium wrote: > Could you do a testRunner.waitUntilDone() here to avoid exiting until the gc's > are complete? That has already been done. In the onload event we call the "load" function. That makes sure to waitUntilDone and calls startTest via the event loop. https://codereview.chromium.org/220203005/diff/1/LayoutTests/svg/animations/s... File LayoutTests/svg/animations/smil-leak-element-instances.svg (right): https://codereview.chromium.org/220203005/diff/1/LayoutTests/svg/animations/s... LayoutTests/svg/animations/smil-leak-element-instances.svg:38: // can artificially keep objects alive. Therfore, test that need to check On 2014/04/01 13:26:09, wibling-chromium wrote: > Ditto. Done. https://codereview.chromium.org/220203005/diff/1/LayoutTests/svg/animations/s... LayoutTests/svg/animations/smil-leak-element-instances.svg:74: } On 2014/04/01 13:26:09, wibling-chromium wrote: > Also can't we waitUntilDone here to not exit until the gc is complete? Already done in the load function. https://codereview.chromium.org/220203005/diff/1/LayoutTests/svg/animations/s... File LayoutTests/svg/animations/smil-leak-elements.svg (right): https://codereview.chromium.org/220203005/diff/1/LayoutTests/svg/animations/s... LayoutTests/svg/animations/smil-leak-elements.svg:36: // can artificially keep objects alive. Therfore, test that need to check On 2014/04/01 13:26:09, wibling-chromium wrote: > Ditto. Done. https://codereview.chromium.org/220203005/diff/1/Source/heap/ThreadState.h File Source/heap/ThreadState.h (right): https://codereview.chromium.org/220203005/diff/1/Source/heap/ThreadState.h#ne... Source/heap/ThreadState.h:281: void performPendingGC(StackState); On 2014/04/01 13:26:09, wibling-chromium wrote: > NIT: Could be made private. Done. https://codereview.chromium.org/220203005/diff/1/Source/heap/ThreadState.h#ne... Source/heap/ThreadState.h:290: bool forcedForTesting(); On 2014/04/01 13:26:09, wibling-chromium wrote: > NIT: Could be made private. This one is called from Heap. We could make heap a friend to hide some of these. However, that goes for a lot of stuff here, so let's do that as a separate change?
On 2014/04/01 13:43:11, Mads Ager (chromium) wrote: > On 2014/04/01 13:37:27, haraken wrote: > > I'm not happy to introduce an asynchronous gc() mechanism into layout tests, > but > > I'm getting to conclude that we cannot avoid it: > > > > - As long as we use conservative stack scanning, we need to trigger a GC at > the > > end of an event loop to get deterministic results. > > - This means that we need to use an asynchronous gc() in layout tests. > > > > So, basically I agree with this CL. However, we might want to consider a > couple > > of things to improve the situation: > > > > - In the oilpan world, nothing is guaranteed about a synchronous gc(). Garbage > > might be collected but might not be collected. Leaving the potential flakiness > > is not allowed in layout tests, so we'll need to completely deprecate > > synchronous gc()s when shipping oilpan. I don't think it's acceptable to take > an > > approach where we rewrite tests incrementally every time we hit some > flakiness. > > I'd propose to rewrite all tests in a reasonable timeline. > > > > - We don't want to duplicate collectGarbage() methods in a lot of tests. Shall > > we implement testRunner.gc(callback)? Then we can do the deprecation in the > > following steps: > > > > (1) Rename gc() to gcDeprecated(). This includes gc() in js-test.js and other > > gc()s written in a test file directly. > > (2) Replace all gcDeprecated() with testRunner.gc(callback). > > (3) Remove gcDeprecated() from layout tests. > > > > What do you think? > > We definitely need to rewrite all of these to get deterministic results. > However, we need to start somewhere. I have added this to js-test.js for now > which is used by a lot of tests so it should cover most cases. As follow-up > changes we should continue the replacement of gc() with > collectGarbage(callback). I will keep that going as a background task. I agree with doing the work incrementally, but we should avoid messing up the code base. Given that there are a substantial amount of tests that don't include js-test.js (there are already 2 tests in this CL:-), I think it would be better to implement the gc() method in testRunner. Then we don't need to duplicate the gc() method every time we find tests that don't include js-test.js.
On 2014/04/01 14:25:40, haraken wrote: > On 2014/04/01 13:43:11, Mads Ager (chromium) wrote: > > On 2014/04/01 13:37:27, haraken wrote: > > > I'm not happy to introduce an asynchronous gc() mechanism into layout tests, > > but > > > I'm getting to conclude that we cannot avoid it: > > > > > > - As long as we use conservative stack scanning, we need to trigger a GC at > > the > > > end of an event loop to get deterministic results. > > > - This means that we need to use an asynchronous gc() in layout tests. > > > > > > So, basically I agree with this CL. However, we might want to consider a > > couple > > > of things to improve the situation: > > > > > > - In the oilpan world, nothing is guaranteed about a synchronous gc(). > Garbage > > > might be collected but might not be collected. Leaving the potential > flakiness > > > is not allowed in layout tests, so we'll need to completely deprecate > > > synchronous gc()s when shipping oilpan. I don't think it's acceptable to > take > > an > > > approach where we rewrite tests incrementally every time we hit some > > flakiness. > > > I'd propose to rewrite all tests in a reasonable timeline. > > > > > > - We don't want to duplicate collectGarbage() methods in a lot of tests. > Shall > > > we implement testRunner.gc(callback)? Then we can do the deprecation in the > > > following steps: > > > > > > (1) Rename gc() to gcDeprecated(). This includes gc() in js-test.js and > other > > > gc()s written in a test file directly. > > > (2) Replace all gcDeprecated() with testRunner.gc(callback). > > > (3) Remove gcDeprecated() from layout tests. > > > > > > What do you think? > > > > We definitely need to rewrite all of these to get deterministic results. > > However, we need to start somewhere. I have added this to js-test.js for now > > which is used by a lot of tests so it should cover most cases. As follow-up > > changes we should continue the replacement of gc() with > > collectGarbage(callback). I will keep that going as a background task. > > I agree with doing the work incrementally, but we should avoid messing up the > code base. Given that there are a substantial amount of tests that don't include > js-test.js (there are already 2 tests in this CL:-), I think it would be better > to implement the gc() method in testRunner. Then we don't need to duplicate the > gc() method every time we find tests that don't include js-test.js. I don't know enough about the tests that don't use js-test.js. Probably most of them could. The tests in this changelist that don't are svg files. They cannot use js-test.js but I expect that most other tests can. I'll dive into it a bit more tomorrow. I think the main thing that people are using is GCController.collect directly. We should replace those with collectGarbage(callback). If people use gc() they are already including js-test.js (because that is where it is defined).
On 2014/04/01 14:35:50, Mads Ager (chromium) wrote: > On 2014/04/01 14:25:40, haraken wrote: > > On 2014/04/01 13:43:11, Mads Ager (chromium) wrote: > > > On 2014/04/01 13:37:27, haraken wrote: > > > > I'm not happy to introduce an asynchronous gc() mechanism into layout > tests, > > > but > > > > I'm getting to conclude that we cannot avoid it: > > > > > > > > - As long as we use conservative stack scanning, we need to trigger a GC > at > > > the > > > > end of an event loop to get deterministic results. > > > > - This means that we need to use an asynchronous gc() in layout tests. > > > > > > > > So, basically I agree with this CL. However, we might want to consider a > > > couple > > > > of things to improve the situation: > > > > > > > > - In the oilpan world, nothing is guaranteed about a synchronous gc(). > > Garbage > > > > might be collected but might not be collected. Leaving the potential > > flakiness > > > > is not allowed in layout tests, so we'll need to completely deprecate > > > > synchronous gc()s when shipping oilpan. I don't think it's acceptable to > > take > > > an > > > > approach where we rewrite tests incrementally every time we hit some > > > flakiness. > > > > I'd propose to rewrite all tests in a reasonable timeline. > > > > > > > > - We don't want to duplicate collectGarbage() methods in a lot of tests. > > Shall > > > > we implement testRunner.gc(callback)? Then we can do the deprecation in > the > > > > following steps: > > > > > > > > (1) Rename gc() to gcDeprecated(). This includes gc() in js-test.js and > > other > > > > gc()s written in a test file directly. > > > > (2) Replace all gcDeprecated() with testRunner.gc(callback). > > > > (3) Remove gcDeprecated() from layout tests. > > > > > > > > What do you think? > > > > > > We definitely need to rewrite all of these to get deterministic results. > > > However, we need to start somewhere. I have added this to js-test.js for now > > > which is used by a lot of tests so it should cover most cases. As follow-up > > > changes we should continue the replacement of gc() with > > > collectGarbage(callback). I will keep that going as a background task. > > > > I agree with doing the work incrementally, but we should avoid messing up the > > code base. Given that there are a substantial amount of tests that don't > include > > js-test.js (there are already 2 tests in this CL:-), I think it would be > better > > to implement the gc() method in testRunner. Then we don't need to duplicate > the > > gc() method every time we find tests that don't include js-test.js. > > I don't know enough about the tests that don't use js-test.js. Probably most of > them could. The tests in this changelist that don't are svg files. They cannot > use js-test.js but I expect that most other tests can. I'll dive into it a bit > more tomorrow. I think the main thing that people are using is > GCController.collect directly. We should replace those with > collectGarbage(callback). If people use gc() they are already including > js-test.js (because that is where it is defined). As far as I know, there are many. $ grep -r 'GCController.collect' LayoutTests/ | wc => 168 $ grep -r 'function gc' LayoutTests/ | wc => 83 Probably implementing GCController.collectAllGarbage(callback) might be better than implementing the gc() method in testRunner. Then you can use GCController.collectAllGarbage(callback) in js-test.js as well.
I discussed how to deal with this issue with Blink guys around me.
How about the following plan?
(1) I don't want to block you from landing this CL, so let's just remove the
duplicated collectGarbage() methods from the svg test files and land it. You can
remove the duplication by including js-test.js in the svg test files like this:
<svg ...>
<script xlink:href="../../resources/js-test.js"></script>
...
</svg>
(2) I will implement GCController.collectGarbage(callback).
(3) Let's replace all gc() methods in layout tests with
GCController.collectGarbage(callback). Not only is this replacement necessary
for oilpan, but also this is a good opportunity to stabilize layout tests that
are flaky because of incorrectly used gc() methods. For example, more than 80
tests define 'function gc() {}' in the test files and some of them are actually
not causing any GC. (This is a known issue and I've been thinking that we need
to fix the flakiness at some point.)
However, the problem is that there are too many tests to be rewritten. As far as
I grep, about 650 tests are using gc() or GCController.collect(). So I'm
planning to announce it to blink-dev@ and ask for help of internal/external
contributors. Given that Blink/WebKit has completed this kind of huge
refactoring in this approach, I hope this will work fine this time as well :)
What do you think?
On 2014/04/02 05:11:22, haraken wrote:
> I discussed how to deal with this issue with Blink guys around me.
>
> How about the following plan?
>
> (1) I don't want to block you from landing this CL, so let's just remove the
> duplicated collectGarbage() methods from the svg test files and land it. You
can
> remove the duplication by including js-test.js in the svg test files like
this:
>
> <svg ...>
> <script xlink:href="../../resources/js-test.js"></script>
> ...
> </svg>
Cool! Didn't know that. Thanks!
> (2) I will implement GCController.collectGarbage(callback).
>
> (3) Let's replace all gc() methods in layout tests with
> GCController.collectGarbage(callback). Not only is this replacement necessary
> for oilpan, but also this is a good opportunity to stabilize layout tests that
> are flaky because of incorrectly used gc() methods. For example, more than 80
> tests define 'function gc() {}' in the test files and some of them are
actually
> not causing any GC. (This is a known issue and I've been thinking that we need
> to fix the flakiness at some point.)
Sounds awesome and I will push on this as well (in between Node hierarchy
changes it will be nice to do something lighter as well.) :)
> However, the problem is that there are too many tests to be rewritten. As far
as
> I grep, about 650 tests are using gc() or GCController.collect(). So I'm
> planning to announce it to blink-dev@ and ask for help of internal/external
> contributors. Given that Blink/WebKit has completed this kind of huge
> refactoring in this approach, I hope this will work fine this time as well :)
>
> What do you think?
I think that all sounds great. Thanks Haraken! :)
Patch set updated to remove duplication. PTAL
lgtm
LGTM. I think we can improve the CL in a follow-up (I'll try). - Implement GCController.collectGarbage(callback). - Instead of setting up multiple event loops using setTimeout(), we should just trigger multiple GCs at the end of the current event loop. https://codereview.chromium.org/220203005/diff/40001/Source/heap/ThreadState.h File Source/heap/ThreadState.h (right): https://codereview.chromium.org/220203005/diff/40001/Source/heap/ThreadState.... Source/heap/ThreadState.h:554: bool m_forcedForTesting; m_forcePreciseGCForTesting ?
https://codereview.chromium.org/220203005/diff/40001/Source/heap/ThreadState.h File Source/heap/ThreadState.h (right): https://codereview.chromium.org/220203005/diff/40001/Source/heap/ThreadState.... Source/heap/ThreadState.h:554: bool m_forcedForTesting; On 2014/04/02 06:13:33, haraken wrote: > > m_forcePreciseGCForTesting ? Done.
The CQ bit was checked by ager@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ager@chromium.org/220203005/60001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: tryserver.blink on linux_blink_rel
The CQ bit was checked by ager@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ager@chromium.org/220203005/80001
The CQ bit was unchecked by ager@chromium.org
The CQ bit was checked by ager@chromium.org
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/ager@chromium.org/220203005/80001
Message was sent while issue was closed.
Change committed as 170648 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
