Simple Cache: sync disk cache perftests carefully.

net/disk_cache/disk_cache_perftest.cc

 // Copyright (c) 2011 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include <limits>
 #include <string>
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/files/file_enumerator.h"
 #include "base/files/file_path.h"
 #include "base/hash.h"
 #include "base/process/process_metrics.h"
 #include "base/strings/string_util.h"
 #include "base/test/perf_time_logger.h"
 #include "base/test/test_file_util.h"
 #include "base/threading/thread.h"
 #include "net/base/cache_type.h"
 #include "net/base/io_buffer.h"
 #include "net/base/net_errors.h"
 #include "net/base/test_completion_callback.h"
 #include "net/disk_cache/blockfile/backend_impl.h"
 #include "net/disk_cache/blockfile/block_files.h"
 #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"
 
 using base::Time;
 
 namespace {
 
 size_t MaybeGetMaxFds() {
 #if defined(OS_POSIX)
   return base::GetMaxFds();
@@ skipping 191 matching lines @@
 #endif
 
   DisableFirstCleanup();
   InitCache();
 }
 
 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();
   EXPECT_TRUE(
       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) {
   CacheBackendPerformance();
 }
 
 TEST_F(DiskCachePerfTest, SimpleCacheBackendPerformance) {
   SetSimpleCacheMode();
   CacheBackendPerformance();
 }
@@ skipping 39 matching lines @@
     files.DeleteBlock(address[entry], false);
     EXPECT_TRUE(
         files.CreateBlock(disk_cache::RANKINGS, BlockSize(), &address[entry]));
   }
 
   timer2.Done();
   base::MessageLoop::current()->RunUntilIdle();
 }
 
 }  // namespace