Simple Cache: sync disk cache perftests carefully.

net/disk_cache/disk_cache_perftest.cc

Index: net/disk_cache/disk_cache_perftest.cc
diff --git a/net/disk_cache/disk_cache_perftest.cc b/net/disk_cache/disk_cache_perftest.cc
index 759515a5f4ddbf7df7246634672629f639a93b64..29bebbce5db0a88c03f600b43be0582d02d8ac7b 100644
--- a/net/disk_cache/disk_cache_perftest.cc
+++ b/net/disk_cache/disk_cache_perftest.cc
@@ -24,6 +24,7 @@
 #include "net/disk_cache/disk_cache.h"
 #include "net/disk_cache/disk_cache_test_base.h"
 #include "net/disk_cache/disk_cache_test_util.h"
+#include "net/disk_cache/simple/simple_backend_impl.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "testing/platform_test.h"
 
@@ -235,11 +236,18 @@ void DiskCachePerfTest::CacheBackendPerformance() {
   InitCache();
   EXPECT_TRUE(TimeWrite());
 
+  base::MessageLoop::current()->RunUntilIdle();

[Randy Smith (Not in Mondays), 2016/05/26 18:26:25]

Why is this needed?  I'd think that FlushWorkerPoolForTesting() will behave
correctly either way (we're not testing its performance) and the suffixed
RunUntilIdle() should allow everything to clear out for the following
performance test.  So this feels a bit like a ritual RunUntilIdle() insertion;
is there something I'm not understanding?

[gavinp, 2016/05/26 19:03:29]

You're right; I was just anxious because this has been required in disk cache
tests before; essentially the tests post a task that bounces off of the worker
pool before finishing off in the worker pool, so both run until idle's appear
neccessary. This test seems to run fine without the extra RunUntilIdle though,
so I've removed it.

+  disk_cache::SimpleBackendImpl::FlushWorkerPoolForTesting();
+  base::MessageLoop::current()->RunUntilIdle();
+
   ResetAndEvictSystemDiskCache();
   EXPECT_TRUE(TimeRead(WhatToRead::HEADERS_ONLY,
                        "Read disk cache headers only (cold)"));
   EXPECT_TRUE(TimeRead(WhatToRead::HEADERS_ONLY,
                        "Read disk cache headers only (warm)"));
+
+  base::MessageLoop::current()->RunUntilIdle();

[Randy Smith (Not in Mondays), 2016/05/26 18:26:25]

I'm a bit more worried about this one, as its placement implies that there's
stuff "left over" from the above performance tests, which is worrying because it
implies that what we're actually measuring the performance of depends on what
finishes during the tests and what after.  Having said that, at a conceptual
level this is the same question as above: What performance/correctness goal does
this line support?  I don't understand it, and I'm a believer in "if you don't
understand it, it's dangerous" :-} :-|.

[gavinp, 2016/05/26 19:03:29]

The Close() operations after each of the reads needs to complete, otherwise the
evictions from the cache below will fail. That's asserted on (since the test is
pretty useless without an evicted entry), so we have a problem if that happens.

The requirement that all FDs be closed before an eviction from the system cache
is novel to windows, hence it was doing more testing on windows that uncovered
this problem.

So it's not as much a precondition for the next test as it is a precondition for
the cache evictions, which are themselves a precondition of the next test.

+  disk_cache::SimpleBackendImpl::FlushWorkerPoolForTesting();
   base::MessageLoop::current()->RunUntilIdle();
 
   ResetAndEvictSystemDiskCache();
@@ -247,7 +255,6 @@ void DiskCachePerfTest::CacheBackendPerformance() {
       TimeRead(WhatToRead::HEADERS_AND_BODY, "Read disk cache entries (cold)"));
   EXPECT_TRUE(
       TimeRead(WhatToRead::HEADERS_AND_BODY, "Read disk cache entries (warm)"));

[Randy Smith (Not in Mondays), 2016/05/26 18:26:25]

What's the larger context in which this test is run?  I tend to believe that
tests should make sure to setup whatever environment they need as opposed to
relying on good citizenship of other tests cleaning up after themselves, but
cleaning up after oneself is still courteous.  Should there be a
RunUntilIdle()/Flush/RunUntilIdle() here as well?

[gavinp, 2016/05/26 19:03:29]

Ah, we don't tear down the worker pool after tests, so they kind of finish over
each other, for better or worse. I've added this back, you're right, good to
have.

-  base::MessageLoop::current()->RunUntilIdle();
 }
 
 TEST_F(DiskCachePerfTest, CacheBackendPerformance) {