page cycler: Use buffer-cache priming code on all systems.

page cycler: Use buffer-cache priming code on all systems.

I noticed that the first time I run the page cycler the test numbers
are worse, probably because the test pages are not in cache.  We
already have this code on Mac and I don't see why it shouldn't apply
to all systems.

Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=47650

[jorlow, 2010-05-17 15:28:44]

Seems reasonable to me, but I don't think I have the authority to LGTM this.

[Mark Mentovai, 2010-05-17 15:53:06]

http://codereview.chromium.org/2096005/diff/1/2
File chrome/test/page_cycler/page_cycler_test.cc (right):

http://codereview.chromium.org/2096005/diff/1/2#newcode24
chrome/test/page_cycler/page_cycler_test.cc:24: #include <errno.h>
I don’t see errno used in this file at all.

http://codereview.chromium.org/2096005/diff/1/2#newcode148
chrome/test/page_cycler/page_cycler_test.cc:148: int fd =
open(path.value().c_str(), O_RDONLY);
I don’t think this will work on Windows.

http://codereview.chromium.org/2096005/diff/1/2#newcode151
chrome/test/page_cycler/page_cycler_test.cc:151: while (HANDLE_EINTR(read(fd,
buf, sizeof(buf))) > 0) {
Move buf’s declaration here.

[Evan Martin, 2010-05-17 15:57:02]

On 2010/05/17 15:53:06, Mark Mentovai wrote:

Hey, shouldn't you have caught these in the initial review?  :P


> chrome/test/page_cycler/page_cycler_test.cc:24: #include <errno.h>
> I don’t see errno used in this file at all.

It looks like it was maybe added for handle_eintr, but that #includes it itself.

> http://codereview.chromium.org/2096005/diff/1/2#newcode148
> chrome/test/page_cycler/page_cycler_test.cc:148: int fd =
> open(path.value().c_str(), O_RDONLY);
> I don’t think this will work on Windows.

I had hoped the trybot would help, but it's been compiling for nearly an hour
and still hasn't finished on Windows.  I can do clean builds on Linux in under
ten minutes.  I hate our build.  :(

Thanks for the quick feedback, I mail you again once I've verified the change
works on Windows.

[Mark Mentovai, 2010-05-17 16:01:10]

Evan wrote:
> Hey, shouldn't you have caught these in the initial review?  :P

I shoulda caught buf, but I bet this file used errno before we had PLOG.

buf is more like a “submarine review comment.” :)

Mark

[Evan Martin, 2010-05-18 12:59:51]

Here's a new cross-platform implementation.

[Evan Martin, 2010-05-19 09:27:55]

Ping.

[Evan Martin, 2010-05-19 10:37:19]

+jochen, who is in my time zone :)

[jorlow, 2010-05-19 10:41:49]

LGTM

http://codereview.chromium.org/2096005/diff/7001/8001
File base/file_util.cc (right):

http://codereview.chromium.org/2096005/diff/7001/8001#newcode189
base/file_util.cc:189: if (contents)
This feels dirty to me.  I understand what you're doing here and that you don't
want to copy/paste code.  I understand that there's almost no chance of this
having a performance impact even though it's inside the loop.  And so on.  But
it just feels a bit dirty.

[Mark Mentovai, 2010-05-19 14:00:27]

LGTM.

(Sorry, I was out yesterday.)

http://codereview.chromium.org/2096005/diff/7001/8001
File base/file_util.cc (right):

http://codereview.chromium.org/2096005/diff/7001/8001#newcode189
base/file_util.cc:189: if (contents)
This does not feel dirty to me at all.