Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
292763002:
Oilpan: simplify ImageBitmapTest heap clearing. (Closed)
|
Created:
6 years, 7 months ago by sof Modified:
6 years, 7 months ago CC:
blink-reviews, oilpan-reviews Base URL:
https://chromium.googlesource.com/chromium/blink.git@master Visibility:
Public. |
DescriptionOilpan: simplify ImageBitmapTest heap clearing.
For ImageBitmapLiveResourcePriority, use a Persistent to hold onto a
local ImageBitmap object rather than set up and rely on a full stack
scan.
R=wibling@chromium.org,haraken@chromium.org,ager@chromium.org
BUG=
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=174302
Patch Set 1 #
Total comments: 6
Patch Set 2 : Add needed ALLOW_UNUSED attr decls #Patch Set 3 : Tweak position of collectGarbage() call #
Total comments: 6
Patch Set 4 : Consistently use Persistents #Messages
Total messages: 28 (0 generated)
https://codereview.chromium.org/292763002/diff/1/Source/core/frame/ImageBitma... File Source/core/frame/ImageBitmapTest.cpp (right): https://codereview.chromium.org/292763002/diff/1/Source/core/frame/ImageBitma... Source/core/frame/ImageBitmapTest.cpp:165: Heap::collectGarbage(ThreadState::NoHeapPointersOnStack); hmm, I'm not quite sure if this change addresses the test failure. Currently there is a raw pointer on stack, but we are using Heap::collectGarbage(HeapPointersOnStack) to prevent the on-stack raw pointer from getting collected. In other words, the on-stack raw pointer shouldn't be collected by the Heap::collectGarbage. If this observation is correct, this CL won't solve the problem...
https://codereview.chromium.org/292763002/diff/1/Source/core/frame/ImageBitma... File Source/core/frame/ImageBitmapTest.cpp (right): https://codereview.chromium.org/292763002/diff/1/Source/core/frame/ImageBitma... Source/core/frame/ImageBitmapTest.cpp:165: Heap::collectGarbage(ThreadState::NoHeapPointersOnStack); On 2014/05/19 13:28:02, haraken wrote: > > hmm, I'm not quite sure if this change addresses the test failure. > > Currently there is a raw pointer on stack, but we are using > Heap::collectGarbage(HeapPointersOnStack) to prevent the on-stack raw pointer > from getting collected. In other words, the on-stack raw pointer shouldn't be > collected by the Heap::collectGarbage. If this observation is correct, this CL > won't solve the problem... The only object reference we care about is a Persistent outside this block, so why would it not work out?
lgtm https://codereview.chromium.org/292763002/diff/1/Source/core/frame/ImageBitma... File Source/core/frame/ImageBitmapTest.cpp (right): https://codereview.chromium.org/292763002/diff/1/Source/core/frame/ImageBitma... Source/core/frame/ImageBitmapTest.cpp:165: Heap::collectGarbage(ThreadState::NoHeapPointersOnStack); On 2014/05/19 13:28:02, haraken wrote: > > hmm, I'm not quite sure if this change addresses the test failure. > > Currently there is a raw pointer on stack, but we are using > Heap::collectGarbage(HeapPointersOnStack) to prevent the on-stack raw pointer > from getting collected. In other words, the on-stack raw pointer shouldn't be > collected by the Heap::collectGarbage. If this observation is correct, this CL > won't solve the problem... As I understand it the assumption was that doing a conservative GC could keep the a dead imageBitmap alive. By not doing the conservative GC this should not happen.
https://codereview.chromium.org/292763002/diff/1/Source/core/frame/ImageBitma... File Source/core/frame/ImageBitmapTest.cpp (right): https://codereview.chromium.org/292763002/diff/1/Source/core/frame/ImageBitma... Source/core/frame/ImageBitmapTest.cpp:165: Heap::collectGarbage(ThreadState::NoHeapPointersOnStack); On 2014/05/19 13:31:30, sof wrote: > On 2014/05/19 13:28:02, haraken wrote: > > > > hmm, I'm not quite sure if this change addresses the test failure. > > > > Currently there is a raw pointer on stack, but we are using > > Heap::collectGarbage(HeapPointersOnStack) to prevent the on-stack raw pointer > > from getting collected. In other words, the on-stack raw pointer shouldn't be > > collected by the Heap::collectGarbage. If this observation is correct, this CL > > won't solve the problem... > > The only object reference we care about is a Persistent outside this block, so > why would it not work out? Sorry, you're right. It might work. I thought that the issue is that before this CL, the on-stack raw pointer is prematurely collected by Heap::collectGarbage. However, it cannot happen because the Heap::collectGarbage is called with the HeapPointersOnStack option, which does conservative scanning and keeps the on-stack raw pointer alive. However, I understand that the issue can exist somewhere else. For example, the issue can be that the conservative scanning of the Heap::collectGarbage conservatively protects things that doesn't need to be protected and it can lead to the failure. If so, it's possible that this change fixes the issue. Nit: You can use Heap::collectAllGarbage() instead.
LGTM.
LGTM, thanks! https://codereview.chromium.org/292763002/diff/1/Source/core/frame/ImageBitma... File Source/core/frame/ImageBitmapTest.cpp (right): https://codereview.chromium.org/292763002/diff/1/Source/core/frame/ImageBitma... Source/core/frame/ImageBitmapTest.cpp:165: Heap::collectGarbage(ThreadState::NoHeapPointersOnStack); Consider moving this to right after the block instead. That way it is impossible to start operating on the raw pointers inside of the block after they have been collected.
https://codereview.chromium.org/292763002/diff/1/Source/core/frame/ImageBitma... File Source/core/frame/ImageBitmapTest.cpp (right): https://codereview.chromium.org/292763002/diff/1/Source/core/frame/ImageBitma... Source/core/frame/ImageBitmapTest.cpp:165: Heap::collectGarbage(ThreadState::NoHeapPointersOnStack); On 2014/05/19 13:52:46, Mads Ager (chromium) wrote: > Consider moving this to right after the block instead. That way it is impossible > to start operating on the raw pointers inside of the block after they have been > collected. ok, done that (also had to do some ALLOW_UNUSED decorations to get it to compile across platforms.) haraken: I did consider using collectAllGarbage(), but it seemed a bit too expensive when wanting to flush just 4/5 objects. Hope that's ok.
Still LGTM, wondering about the use of ASSERT_EQ vs EXPECT_EQ in these tests. We are using EXPECT_EQ in the heap unit tests. But not sure if there is a general rule for when we use what? https://codereview.chromium.org/292763002/diff/40001/Source/core/frame/ImageB... File Source/core/frame/ImageBitmapTest.cpp (right): https://codereview.chromium.org/292763002/diff/40001/Source/core/frame/ImageB... Source/core/frame/ImageBitmapTest.cpp:151: RefPtrWillBeRawPtr<ImageBitmap> imageBitmapNoCrop ALLOW_UNUSED = ImageBitmap::create(imageNoCrop.get(), IntRect(0, 0, m_bitmap.width(), m_bitmap.height())); I wonder if all the ASSERT_EQ should be EXPECT_EQ? It does seem rather silly if the test doesn't check anything in release mode?
https://codereview.chromium.org/292763002/diff/40001/Source/core/frame/ImageB... File Source/core/frame/ImageBitmapTest.cpp (right): https://codereview.chromium.org/292763002/diff/40001/Source/core/frame/ImageB... Source/core/frame/ImageBitmapTest.cpp:151: RefPtrWillBeRawPtr<ImageBitmap> imageBitmapNoCrop ALLOW_UNUSED = ImageBitmap::create(imageNoCrop.get(), IntRect(0, 0, m_bitmap.width(), m_bitmap.height())); On 2014/05/19 14:22:47, Mads Ager (chromium) wrote: > I wonder if all the ASSERT_EQ should be EXPECT_EQ? It does seem rather silly if > the test doesn't check anything in release mode? While ASSERT_EQ() may hide some other, later failures in this test, isn't this ok (cf. https://code.google.com/p/googletest/wiki/Primer#Assertions ) ?
https://codereview.chromium.org/292763002/diff/40001/Source/core/frame/ImageB... File Source/core/frame/ImageBitmapTest.cpp (right): https://codereview.chromium.org/292763002/diff/40001/Source/core/frame/ImageB... Source/core/frame/ImageBitmapTest.cpp:151: RefPtrWillBeRawPtr<ImageBitmap> imageBitmapNoCrop ALLOW_UNUSED = ImageBitmap::create(imageNoCrop.get(), IntRect(0, 0, m_bitmap.width(), m_bitmap.height())); On 2014/05/19 14:33:29, sof wrote: > On 2014/05/19 14:22:47, Mads Ager (chromium) wrote: > > I wonder if all the ASSERT_EQ should be EXPECT_EQ? It does seem rather silly > if > > the test doesn't check anything in release mode? > > While ASSERT_EQ() may hide some other, later failures in this test, isn't this > ok (cf. https://code.google.com/p/googletest/wiki/Primer#Assertions ) ? I don't really care much about the distinction on that page. However, it seems that there is another difference. These variables are definitely not unused. They are used in the code that checks below. I believe that EXPECT_EQ will check in release mode as well which would allow us to get rid of the ALLOW_UNUSED on these variables that should not be unused in this test. I believe we had similar issues in the heap_unit_tests which was the main reason for switching to using EXPECT_EQ.
On 2014/05/19 15:00:48, Mads Ager (chromium) wrote: > https://codereview.chromium.org/292763002/diff/40001/Source/core/frame/ImageB... > File Source/core/frame/ImageBitmapTest.cpp (right): > > https://codereview.chromium.org/292763002/diff/40001/Source/core/frame/ImageB... > Source/core/frame/ImageBitmapTest.cpp:151: RefPtrWillBeRawPtr<ImageBitmap> > imageBitmapNoCrop ALLOW_UNUSED = ImageBitmap::create(imageNoCrop.get(), > IntRect(0, 0, m_bitmap.width(), m_bitmap.height())); > On 2014/05/19 14:33:29, sof wrote: > > On 2014/05/19 14:22:47, Mads Ager (chromium) wrote: > > > I wonder if all the ASSERT_EQ should be EXPECT_EQ? It does seem rather silly > > if > > > the test doesn't check anything in release mode? > > > > While ASSERT_EQ() may hide some other, later failures in this test, isn't this > > ok (cf. https://code.google.com/p/googletest/wiki/Primer#Assertions ) ? > > I don't really care much about the distinction on that page. However, it seems > that there is another difference. These variables are definitely not unused. > They are used in the code that checks below. I believe that EXPECT_EQ will check > in release mode as well which would allow us to get rid of the ALLOW_UNUSED on > these variables that should not be unused in this test. I believe we had similar > issues in the heap_unit_tests which was the main reason for switching to using > EXPECT_EQ. These boil down to the same thing expect for the fact that one is fatal and the other one is not as that page points out. I wonder if the fatal part of one of them makes the compiler warn that the variables are "potentially" unused. That's probably what it is?
https://codereview.chromium.org/292763002/diff/40001/Source/core/frame/ImageB... File Source/core/frame/ImageBitmapTest.cpp (right): https://codereview.chromium.org/292763002/diff/40001/Source/core/frame/ImageB... Source/core/frame/ImageBitmapTest.cpp:151: RefPtrWillBeRawPtr<ImageBitmap> imageBitmapNoCrop ALLOW_UNUSED = ImageBitmap::create(imageNoCrop.get(), IntRect(0, 0, m_bitmap.width(), m_bitmap.height())); On 2014/05/19 15:00:48, Mads Ager (chromium) wrote: > On 2014/05/19 14:33:29, sof wrote: > > On 2014/05/19 14:22:47, Mads Ager (chromium) wrote: > > > I wonder if all the ASSERT_EQ should be EXPECT_EQ? It does seem rather silly > > if > > > the test doesn't check anything in release mode? > > > > While ASSERT_EQ() may hide some other, later failures in this test, isn't this > > ok (cf. https://code.google.com/p/googletest/wiki/Primer#Assertions ) ? > > I don't really care much about the distinction on that page. However, it seems > that there is another difference. These variables are definitely not unused. > They are used in the code that checks below. I believe that EXPECT_EQ will check > in release mode as well which would allow us to get rid of the ALLOW_UNUSED on > these variables that should not be unused in this test. I believe we had similar > issues in the heap_unit_tests which was the main reason for switching to using > EXPECT_EQ. ok, i see. The variables are unused (imageBitmapNoCrop vs imageNoCrop, for instance.)
PS: Please go ahead and land this. I'm just trying to understand what is going on with the UNUSED annotations. We can change that independently later if we decide to.
https://codereview.chromium.org/292763002/diff/40001/Source/core/frame/ImageB... File Source/core/frame/ImageBitmapTest.cpp (right): https://codereview.chromium.org/292763002/diff/40001/Source/core/frame/ImageB... Source/core/frame/ImageBitmapTest.cpp:151: RefPtrWillBeRawPtr<ImageBitmap> imageBitmapNoCrop ALLOW_UNUSED = ImageBitmap::create(imageNoCrop.get(), IntRect(0, 0, m_bitmap.width(), m_bitmap.height())); On 2014/05/19 15:10:10, sof wrote: > On 2014/05/19 15:00:48, Mads Ager (chromium) wrote: > > On 2014/05/19 14:33:29, sof wrote: > > > On 2014/05/19 14:22:47, Mads Ager (chromium) wrote: > > > > I wonder if all the ASSERT_EQ should be EXPECT_EQ? It does seem rather > silly > > > if > > > > the test doesn't check anything in release mode? > > > > > > While ASSERT_EQ() may hide some other, later failures in this test, isn't > this > > > ok (cf. https://code.google.com/p/googletest/wiki/Primer#Assertions ) ? > > > > I don't really care much about the distinction on that page. However, it seems > > that there is another difference. These variables are definitely not unused. > > They are used in the code that checks below. I believe that EXPECT_EQ will > check > > in release mode as well which would allow us to get rid of the ALLOW_UNUSED on > > these variables that should not be unused in this test. I believe we had > similar > > issues in the heap_unit_tests which was the main reason for switching to using > > EXPECT_EQ. > > ok, i see. The variables are unused (imageBitmapNoCrop vs imageNoCrop, for > instance.) d'oh, yes, I misread that. That raises another concern though: if these are unused, there is nothing that guarantees that they will stay alive with Oilpan. The compiler is free to not have those variables on the stack, nor in registers. Therefore, these unused objects can die at one of the next allocations. So, maybe these should really be in Persistents if it is important that they stay alive for this scope? That could also be a cause of the flakiness. :)
https://codereview.chromium.org/292763002/diff/40001/Source/core/frame/ImageB... File Source/core/frame/ImageBitmapTest.cpp (right): https://codereview.chromium.org/292763002/diff/40001/Source/core/frame/ImageB... Source/core/frame/ImageBitmapTest.cpp:151: RefPtrWillBeRawPtr<ImageBitmap> imageBitmapNoCrop ALLOW_UNUSED = ImageBitmap::create(imageNoCrop.get(), IntRect(0, 0, m_bitmap.width(), m_bitmap.height())); On 2014/05/19 15:15:02, Mads Ager (chromium) wrote: > On 2014/05/19 15:10:10, sof wrote: > > On 2014/05/19 15:00:48, Mads Ager (chromium) wrote: > > > On 2014/05/19 14:33:29, sof wrote: > > > > On 2014/05/19 14:22:47, Mads Ager (chromium) wrote: > > > > > I wonder if all the ASSERT_EQ should be EXPECT_EQ? It does seem rather > > silly > > > > if > > > > > the test doesn't check anything in release mode? > > > > > > > > While ASSERT_EQ() may hide some other, later failures in this test, isn't > > this > > > > ok (cf. https://code.google.com/p/googletest/wiki/Primer#Assertions ) ? > > > > > > I don't really care much about the distinction on that page. However, it > seems > > > that there is another difference. These variables are definitely not unused. > > > They are used in the code that checks below. I believe that EXPECT_EQ will > > check > > > in release mode as well which would allow us to get rid of the ALLOW_UNUSED > on > > > these variables that should not be unused in this test. I believe we had > > similar > > > issues in the heap_unit_tests which was the main reason for switching to > using > > > EXPECT_EQ. > > > > ok, i see. The variables are unused (imageBitmapNoCrop vs imageNoCrop, for > > instance.) > > d'oh, yes, I misread that. > > That raises another concern though: if these are unused, there is nothing that > guarantees that they will stay alive with Oilpan. The compiler is free to not > have those variables on the stack, nor in registers. Therefore, these unused > objects can die at one of the next allocations. So, maybe these should really be > in Persistents if it is important that they stay alive for this scope? That > could also be a cause of the flakiness. :) That could be, but the failure reported is outside this block & suggesting that they haven't been collected.
On 2014/05/19 15:17:03, sof wrote: > https://codereview.chromium.org/292763002/diff/40001/Source/core/frame/ImageB... > File Source/core/frame/ImageBitmapTest.cpp (right): > > https://codereview.chromium.org/292763002/diff/40001/Source/core/frame/ImageB... > Source/core/frame/ImageBitmapTest.cpp:151: RefPtrWillBeRawPtr<ImageBitmap> > imageBitmapNoCrop ALLOW_UNUSED = ImageBitmap::create(imageNoCrop.get(), > IntRect(0, 0, m_bitmap.width(), m_bitmap.height())); > On 2014/05/19 15:15:02, Mads Ager (chromium) wrote: > > On 2014/05/19 15:10:10, sof wrote: > > > On 2014/05/19 15:00:48, Mads Ager (chromium) wrote: > > > > On 2014/05/19 14:33:29, sof wrote: > > > > > On 2014/05/19 14:22:47, Mads Ager (chromium) wrote: > > > > > > I wonder if all the ASSERT_EQ should be EXPECT_EQ? It does seem rather > > > silly > > > > > if > > > > > > the test doesn't check anything in release mode? > > > > > > > > > > While ASSERT_EQ() may hide some other, later failures in this test, > isn't > > > this > > > > > ok (cf. https://code.google.com/p/googletest/wiki/Primer#Assertions ) ? > > > > > > > > I don't really care much about the distinction on that page. However, it > > seems > > > > that there is another difference. These variables are definitely not > unused. > > > > They are used in the code that checks below. I believe that EXPECT_EQ will > > > check > > > > in release mode as well which would allow us to get rid of the > ALLOW_UNUSED > > on > > > > these variables that should not be unused in this test. I believe we had > > > similar > > > > issues in the heap_unit_tests which was the main reason for switching to > > using > > > > EXPECT_EQ. > > > > > > ok, i see. The variables are unused (imageBitmapNoCrop vs imageNoCrop, for > > > instance.) > > > > d'oh, yes, I misread that. > > > > That raises another concern though: if these are unused, there is nothing that > > guarantees that they will stay alive with Oilpan. The compiler is free to not > > have those variables on the stack, nor in registers. Therefore, these unused > > objects can die at one of the next allocations. So, maybe these should really > be > > in Persistents if it is important that they stay alive for this scope? That > > could also be a cause of the flakiness. :) > > That could be, but the failure reported is outside this block & suggesting that > they haven't been collected. That makes sense. I think we should put those unused variables in Persistents and remove the UNUSED annotation. That way we are sure that the lifetime of those objects are as they were before. They *are* alive for all of the scope and they *will* die at the GC with NoHeapPointersOnStack. That will make it fully deterministic which a test should be. :)
On 2014/05/19 15:19:52, Mads Ager (chromium) wrote: > On 2014/05/19 15:17:03, sof wrote: > > > https://codereview.chromium.org/292763002/diff/40001/Source/core/frame/ImageB... > > File Source/core/frame/ImageBitmapTest.cpp (right): > > > > > https://codereview.chromium.org/292763002/diff/40001/Source/core/frame/ImageB... > > Source/core/frame/ImageBitmapTest.cpp:151: RefPtrWillBeRawPtr<ImageBitmap> > > imageBitmapNoCrop ALLOW_UNUSED = ImageBitmap::create(imageNoCrop.get(), > > IntRect(0, 0, m_bitmap.width(), m_bitmap.height())); > > On 2014/05/19 15:15:02, Mads Ager (chromium) wrote: > > > On 2014/05/19 15:10:10, sof wrote: > > > > On 2014/05/19 15:00:48, Mads Ager (chromium) wrote: > > > > > On 2014/05/19 14:33:29, sof wrote: > > > > > > On 2014/05/19 14:22:47, Mads Ager (chromium) wrote: > > > > > > > I wonder if all the ASSERT_EQ should be EXPECT_EQ? It does seem > rather > > > > silly > > > > > > if > > > > > > > the test doesn't check anything in release mode? > > > > > > > > > > > > While ASSERT_EQ() may hide some other, later failures in this test, > > isn't > > > > this > > > > > > ok (cf. https://code.google.com/p/googletest/wiki/Primer#Assertions ) > ? > > > > > > > > > > I don't really care much about the distinction on that page. However, it > > > seems > > > > > that there is another difference. These variables are definitely not > > unused. > > > > > They are used in the code that checks below. I believe that EXPECT_EQ > will > > > > check > > > > > in release mode as well which would allow us to get rid of the > > ALLOW_UNUSED > > > on > > > > > these variables that should not be unused in this test. I believe we had > > > > similar > > > > > issues in the heap_unit_tests which was the main reason for switching to > > > using > > > > > EXPECT_EQ. > > > > > > > > ok, i see. The variables are unused (imageBitmapNoCrop vs imageNoCrop, for > > > > instance.) > > > > > > d'oh, yes, I misread that. > > > > > > That raises another concern though: if these are unused, there is nothing > that > > > guarantees that they will stay alive with Oilpan. The compiler is free to > not > > > have those variables on the stack, nor in registers. Therefore, these unused > > > objects can die at one of the next allocations. So, maybe these should > really > > be > > > in Persistents if it is important that they stay alive for this scope? That > > > could also be a cause of the flakiness. :) > > > > That could be, but the failure reported is outside this block & suggesting > that > > they haven't been collected. > > That makes sense. I think we should put those unused variables in Persistents > and remove the UNUSED annotation. That way we are sure that the lifetime of > those objects are as they were before. They *are* alive for all of the scope and > they *will* die at the GC with NoHeapPointersOnStack. That will make it fully > deterministic which a test should be. :) Thanks, let's close out any such loopholes - uploaded new patchset.
Looks great! Ship it! :)
The CQ bit was checked by sigbjornf@opera.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sigbjornf@opera.com/292763002/60001
Unit tests are showing green, http://build.chromium.org/p/tryserver.blink/builders/linux_blink_oilpan_rel/b...
FYI, CQ is re-trying this CL (attempt #1). Please consider checking whether the failures are real, and report flakes to chrome-troopers@google.com. The failing builders are: mac_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/8157)
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_blink_rel on tryserver.blink (http://build.chromium.org/p/tryserver.blink/builders/mac_blink_rel/builds/8165)
The CQ bit was checked by sigbjornf@opera.com
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/sigbjornf@opera.com/292763002/60001
Message was sent while issue was closed.
Change committed as 174302 |
