Simple Cache: sync disk cache perftests carefully.

Simple Cache: sync disk cache perftests carefully.

The simple cache in particular requires very cautious sync
to ensure prior operations are completed. Existing tests
are more stable, particularly on windows if we do this.

R=rdsmith@chromium.org
BUG=611464
Committed: https://crrev.com/6a121fdb06ebe1ccb9bf2233404a25c8559587ec
Cr-Commit-Position: refs/heads/master@{#396272}

[gavinp, 2016-05-26 16:35:50]

rdsmith: PTAL

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

This is clearly an improvement over what's there, so: LGTM.  But I'd be
interested in the answers to my questions below, and you should feel free to be
inspired to make changes based on those questions if that's appropriate.

https://codereview.chromium.org/2018583002/diff/20001/net/disk_cache/disk_cac...
File net/disk_cache/disk_cache_perftest.cc (right):

https://codereview.chromium.org/2018583002/diff/20001/net/disk_cache/disk_cac...
net/disk_cache/disk_cache_perftest.cc:239:
base::MessageLoop::current()->RunUntilIdle();
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?

https://codereview.chromium.org/2018583002/diff/20001/net/disk_cache/disk_cac...
net/disk_cache/disk_cache_perftest.cc:249:
base::MessageLoop::current()->RunUntilIdle();
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" :-} :-|.

https://codereview.chromium.org/2018583002/diff/20001/net/disk_cache/disk_cac...
net/disk_cache/disk_cache_perftest.cc:257:
TimeRead(WhatToRead::HEADERS_AND_BODY, "Read disk cache entries (warm)"));
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]

Thanks. I'm making a second upload per your suggestions.

https://codereview.chromium.org/2018583002/diff/20001/net/disk_cache/disk_cac...
File net/disk_cache/disk_cache_perftest.cc (right):

https://codereview.chromium.org/2018583002/diff/20001/net/disk_cache/disk_cac...
net/disk_cache/disk_cache_perftest.cc:239:
base::MessageLoop::current()->RunUntilIdle();
On 2016/05/26 18:26:25, Randy Smith - Not in Fridays wrote:
> 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?

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.

https://codereview.chromium.org/2018583002/diff/20001/net/disk_cache/disk_cac...
net/disk_cache/disk_cache_perftest.cc:249:
base::MessageLoop::current()->RunUntilIdle();
On 2016/05/26 18:26:25, Randy Smith - Not in Fridays wrote:
> 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" :-} :-|.

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.

https://codereview.chromium.org/2018583002/diff/20001/net/disk_cache/disk_cac...
net/disk_cache/disk_cache_perftest.cc:257:
TimeRead(WhatToRead::HEADERS_AND_BODY, "Read disk cache entries (warm)"));
On 2016/05/26 18:26:25, Randy Smith - Not in Fridays wrote:
> 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?

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.

[gavinp, 2016-05-26 19:06:32]

The CQ bit was checked by gavinp@chromium.org

[gavinp, 2016-05-26 19:06:33]

The patchset sent to the CQ was uploaded after l-g-t-m from rdsmith@chromium.org
Link to the patchset: https://codereview.chromium.org/2018583002/#ps40001
(title: "be (more|less) superstitious as appropriate")

[commit-bot: I haz the power, 2016-05-26 19:06:49]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/2018583002/40001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/2018583002/40001

[commit-bot: I haz the power, 2016-05-26 20:34:41]

Committed patchset #3 (id:40001)

[commit-bot: I haz the power, 2016-05-26 20:36:11]

Description was changed from

==========
Simple Cache: sync disk cache perftests carefully.

The simple cache in particular requires very cautious sync
to ensure prior operations are completed. Existing tests
are more stable, particularly on windows if we do this.

R=rdsmith@chromium.org
BUG=611464
==========

to

==========
Simple Cache: sync disk cache perftests carefully.

The simple cache in particular requires very cautious sync
to ensure prior operations are completed. Existing tests
are more stable, particularly on windows if we do this.

R=rdsmith@chromium.org
BUG=611464

Committed: https://crrev.com/6a121fdb06ebe1ccb9bf2233404a25c8559587ec
Cr-Commit-Position: refs/heads/master@{#396272}
==========

[commit-bot: I haz the power, 2016-05-26 20:36:12]

Patchset 3 (id:??) landed as
https://crrev.com/6a121fdb06ebe1ccb9bf2233404a25c8559587ec
Cr-Commit-Position: refs/heads/master@{#396272}