Port flush_cache tool.

Port flush_cache tool and perf_tests.

BUG=4160, 4263
Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=6815

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

[Dean McNamee, 2008-11-06 16:24:53]

http://codereview.chromium.org/9639/diff/1/4
File base/file_util.h (right):

http://codereview.chromium.org/9639/diff/1/4#newcode392
Line 392: // to access this file will result in a cold load from the hard drive.
I believe this works, but the comment makes the guarantee sound too strong.  The
kernel is allowed to ignore any of the advisories.  Maybe I'm just being
pedantic.

http://codereview.chromium.org/9639/diff/1/3
File base/file_util_win.cc (right):

http://codereview.chromium.org/9639/diff/1/3#newcode699
Line 699: // these opterations will be slow without the cache.
spelling

http://codereview.chromium.org/9639/diff/1/3#newcode700
Line 700: char buffer[128 * 1024];
128k is pretty big for a stack variable, you have to be careful that you might
exhaust the stack if a thread has a smaller stack or something...

We are only using this for testing right?  Performance doesn't really matter. 
Was there any problems with the original code, why not just keep it?

[please use my chromium address, 2008-11-06 16:35:59]

On 2008/11/06 16:24:53, Dean McNamee wrote:
> http://codereview.chromium.org/9639/diff/1/4
> File base/file_util.h (right):
> 
> http://codereview.chromium.org/9639/diff/1/4#newcode392
> Line 392: // to access this file will result in a cold load from the hard
drive.
> I believe this works, but the comment makes the guarantee sound too strong. 
The
> kernel is allowed to ignore any of the advisories.  Maybe I'm just being
> pedantic.

I will add a slight comment and then let you decide about this.
Windows should respect no-buffering flag.
POSIX operating systems may ignore fadvise, but Linux doesn't. OS X has some
different way to flush cache, and that's why I added fadvise code to
file_util_linux instead of file_util_posix.

> http://codereview.chromium.org/9639/diff/1/3
> File base/file_util_win.cc (right):
> 
> http://codereview.chromium.org/9639/diff/1/3#newcode699
> Line 699: // these opterations will be slow without the cache.
> spelling

Done.

> http://codereview.chromium.org/9639/diff/1/3#newcode700
> Line 700: char buffer[128 * 1024];
> 128k is pretty big for a stack variable, you have to be careful that you might
> exhaust the stack if a thread has a smaller stack or something...
> 
> We are only using this for testing right?  Performance doesn't really matter. 
> Was there any problems with the original code, why not just keep it?

Only for testing and performance testing (net_perftests). But even for
performance test performance of this function doesn't matter, because it's being
run before timer starts. If it should be changed, I see two solutions:
- reading in smaller chunks (1k)
- dynamically allocating buffer

Please decide if I should change this code and if so, how.

[please use my chromium address, 2008-11-06 18:31:41]

I forgot to add my disk_cache_perftest changes. This removes duplication.

[agl, 2008-11-06 19:04:55]

http://codereview.chromium.org/9639/diff/213/216
File base/file_util.h (right):

http://codereview.chromium.org/9639/diff/213/216#newcode393
Line 393: bool EvictFileFromSystemCache(const wchar_t* file);
We probably shouldn't be using wchar_t for filenames anymore, but FilePath
instead.

http://codereview.chromium.org/9639/diff/213/218
File base/file_util_linux.cc (right):

http://codereview.chromium.org/9639/diff/213/218#newcode76
Line 76: int fd = open(WideToUTF8(std::wstring(name)).c_str(), O_RDONLY);
Likewise, FilePath is the way of the future.

http://codereview.chromium.org/9639/diff/213/217
File base/file_util_mac.mm (right):

http://codereview.chromium.org/9639/diff/213/217#newcode31
Line 31: bool EvictFileFromSystemCache(const wchar_t* name) {
Likewise

http://codereview.chromium.org/9639/diff/213/215
File base/file_util_win.cc (right):

http://codereview.chromium.org/9639/diff/213/215#newcode690
Line 690: bool EvictFileFromSystemCache(const wchar_t* name) {
Likewise

[please use my chromium address, 2008-11-06 21:14:55]

Updated to use FilePath.

[agl, 2008-11-11 01:04:00]

LGTM

[please use my chromium address, 2008-11-11 21:14:46]

Patch updated to hopefully fix compile failure.

[please use my chromium address, 2008-11-14 20:28:23]

Updated after patch got reverted (don't know reason yet, just merged changes
from trunk and made it compile on Linux).

[please use my chromium address, 2008-11-18 19:58:12]

On 2008/11/14 20:28:23, phajdan.jr wrote:
> Updated after patch got reverted (don't know reason yet, just merged changes
> from trunk and made it compile on Linux).

Updated to merge changes from trunk.

[please use my chromium address, 2008-11-24 11:37:02]

*ping* *ping* *ping*

This patch is already LGTM-ed. I updated it today to be sure it applies cleanly.
Please look.

[Dean McNamee, 2008-11-24 16:26:59]

http://codereview.chromium.org/9639/diff/1022/1217
File base/file_util_win.cc (right):

http://codereview.chromium.org/9639/diff/1022/1217#newcode711
Line 711: char buffer[128 * 1024];
128k is huge for a stack variable, I'm worried we might run out of stack on some
threads if they're running with small stack sizes.

[brettw, 2008-11-24 16:53:51]

http://codereview.chromium.org/9639/diff/1022/1217
File base/file_util_win.cc (right):

http://codereview.chromium.org/9639/diff/1022/1217#newcode711
Line 711: char buffer[128 * 1024];
I agree. I don't think any lines of this function should have changed from the
old location. You can change code and move code, but please don't do both at the
same time, since it makes it impossible to see what changed.

[please use my chromium address, 2008-11-24 20:29:55]

On 2008/11/24 16:53:51, brettw wrote:
> http://codereview.chromium.org/9639/diff/1022/1217
> File base/file_util_win.cc (right):
> 
> http://codereview.chromium.org/9639/diff/1022/1217#newcode711
> Line 711: char buffer[128 * 1024];
> I agree. I don't think any lines of this function should have changed from the
> old location. You can change code and move code, but please don't do both at
the
> same time, since it makes it impossible to see what changed.

Submitted http://codereview.chromium.org/12407 to separate code move from code
change.

[Dean McNamee, 2008-11-26 00:00:44]

Please merge with recent changes (test_file_util moving to base/, etc).

On 2008/11/24 20:29:55, phajdan.jr wrote:
> On 2008/11/24 16:53:51, brettw wrote:
> > http://codereview.chromium.org/9639/diff/1022/1217
> > File base/file_util_win.cc (right):
> > 
> > http://codereview.chromium.org/9639/diff/1022/1217#newcode711
> > Line 711: char buffer[128 * 1024];
> > I agree. I don't think any lines of this function should have changed from
the
> > old location. You can change code and move code, but please don't do both at
> the
> > same time, since it makes it impossible to see what changed.
> 
> Submitted http://codereview.chromium.org/12407 to separate code move from code
> change.

[please use my chromium address, 2008-11-26 20:03:40]

Merged trunk changes, and merged flush_cache and perf_tests together, because
changes are related to each other.

[Dean McNamee, 2008-11-27 00:36:53]

net_perftests errors:
1: disk_cache_perftest.obj : error LNK2019: unresolved external symbol "bool
__cdecl file_util::EvictFileFromSystemCache(class FilePath const &)"
(?EvictFileFromSystemCache@file_util@@YA_NABVFilePath@@@Z) referenced in
function "private: virtual void __thiscall
DiskCacheTest_CacheBackendPerformance_Test::TestBody(void)"
(?TestBody@DiskCacheTest_CacheBackendPerformance_Test@@EAEXXZ)



2: C:\b\slave\try-win32-3\build\src\chrome\Debug\net_perftests.exe : fatal error
LNK1120: 1 unresolved externals



flush_cache errors:
1: flush_cache.obj : error LNK2019: unresolved external symbol "bool __cdecl
file_util::EvictFileFromSystemCache(class FilePath const &)"
(?EvictFileFromSystemCache@file_util@@YA_NABVFilePath@@@Z) referenced in
function _main



2: C:\b\slave\try-win32-3\build\src\chrome\Debug\flush_cache.exe : fatal error
LNK1120: 1 unresolved externals



base errors:
1: ..\test_file_util_win.cc(93) : error C2664:
'file_util::EvictFileFromSystemCache' : cannot convert parameter 1 from 'const
wchar_t *' to 'const FilePath &'



test_shell errors:
1: javascriptcore_pcre.lib(pcre_xclass.obj) : error LNK2005: "bool __cdecl
kjs_pcre_xclass(int,unsigned char const *)" (?kjs_pcre_xclass@@YA_NHPBE@Z)
already defined in v8_snapshot.lib(pcre_xclass.obj)



2: C:\b\slave\try-win32-3\build\src\chrome\Debug\test_shell.exe : fatal error
LNK1169: one or more multiply defined symbols found



perf_tests errors:
1: visitedlink_perftest.obj : error LNK2019: unresolved external symbol "bool
__cdecl file_util::EvictFileFromSystemCache(class FilePath const &)"
(?EvictFileFromSystemCache@file_util@@YA_NABVFilePath@@@Z) referenced in
function "private: virtual void __thiscall
VisitedLink_TestLoad_Test::TestBody(void)"
(?TestBody@VisitedLink_TestLoad_Test@@EAEXXZ)



2: database_perftest.obj : error LNK2001: unresolved external symbol "bool
__cdecl file_util::EvictFileFromSystemCache(class FilePath const &)"
(?EvictFileFromSystemCache@file_util@@YA_NABVFilePath@@@Z)


On 2008/11/26 20:03:40, phajdan.jr wrote:
> Merged trunk changes, and merged flush_cache and perf_tests together, because
> changes are related to each other.

[please use my chromium address, 2008-11-27 11:47:07]

On 2008/11/27 00:36:53, Dean McNamee wrote:
> test_shell errors:
> 1: javascriptcore_pcre.lib(pcre_xclass.obj) : error LNK2005: "bool __cdecl
> kjs_pcre_xclass(int,unsigned char const *)" (?kjs_pcre_xclass@@YA_NHPBE@Z)
> already defined in v8_snapshot.lib(pcre_xclass.obj)
> 2: C:\b\slave\try-win32-3\build\src\chrome\Debug\test_shell.exe : fatal error
> LNK1169: one or more multiply defined symbols found

All errors except this one fixed. I don't know how to solve one above. It seems
the patch couldn't cause that...

[please use my chromium address, 2008-11-27 12:22:09]

On 2008/11/27 11:47:07, phajdan.jr wrote:
> On 2008/11/27 00:36:53, Dean McNamee wrote:
> > test_shell errors:
> > 1: javascriptcore_pcre.lib(pcre_xclass.obj) : error LNK2005: "bool __cdecl
> > kjs_pcre_xclass(int,unsigned char const *)" (?kjs_pcre_xclass@@YA_NHPBE@Z)
> > already defined in v8_snapshot.lib(pcre_xclass.obj)
> > 2: C:\b\slave\try-win32-3\build\src\chrome\Debug\test_shell.exe : fatal
error
> > LNK1169: one or more multiply defined symbols found
> 
> All errors except this one fixed. I don't know how to solve one above. It
seems
> the patch couldn't cause that...

See
http://groups.google.com/group/chromium-dev/browse_thread/thread/bd6b3c5d07a2...
for more info about this error.

[Dean McNamee, 2008-12-03 11:49:37]

http://codereview.chromium.org/9639/diff/636/860
File chrome/browser/safe_browsing/database_perftest.cc (right):

http://codereview.chromium.org/9639/diff/636/860#newcode48
Line 48: filename = filename.Append(name.value());
Should name actually be a FilePath?  Or is it just intentioned to be a name, and
separate from the path.  Otherwise it seems like FilePath.Append should be able
to take a FilePath?

http://codereview.chromium.org/9639/diff/636/860#newcode280
Line 280: int hostkey = base::RandInt(0, std::numeric_limits<int>::max());
This isn't the same is it, before hostkey could be negative?

http://codereview.chromium.org/9639/diff/636/860#newcode294
Line 294: int key = base::RandInt(0, std::numeric_limits<int>::max());
Same

http://codereview.chromium.org/9639/diff/636/860#newcode415
Line 415: host.host = base::RandInt(0, std::numeric_limits<int>::max());
same

http://codereview.chromium.org/9639/diff/636/860#newcode439
Line 439: int key = base::RandInt(0, std::numeric_limits<int>::max());
same

http://codereview.chromium.org/9639/diff/636/860#newcode529
Line 529: SafeBrowsingDatabaseTest
db(FilePath(FILE_PATH_LITERAL("SafeBrowsing500K")));
I think you should just make these narrow, I guess then the problem is then how
to append because it will be narrow in some cases or wide in another... It seems
strange to make them FilePath though.

http://codereview.chromium.org/9639/diff/636/857
File chrome/test/perf/perftests.cc (right):

http://codereview.chromium.org/9639/diff/636/857#newcode18
Line 18: base::AtExitManager exit_manager;
I think maybe it makes sense to have the AtExit be the first thing, but I'm not
sure it's important.

http://codereview.chromium.org/9639/diff/636/858
File chrome/test/perf/perftests.scons (right):

http://codereview.chromium.org/9639/diff/636/858#newcode13
Line 13: env.SConscript([
Curious of SConscript vs ApplySConscript

http://codereview.chromium.org/9639/diff/636/858#newcode22
Line 22: env.Append(
Why the change to Append? Not that it matters, just curious.

[please use my chromium address, 2008-12-03 16:57:11]

On 2008/12/03 11:49:37, Dean McNamee wrote:
> http://codereview.chromium.org/9639/diff/636/860#newcode280
> Line 280: int hostkey = base::RandInt(0, std::numeric_limits<int>::max());
> This isn't the same is it, before hostkey could be negative?

Changed to preserve behavior.

> http://codereview.chromium.org/9639/diff/636/857#newcode18
> Line 18: base::AtExitManager exit_manager;
> I think maybe it makes sense to have the AtExit be the first thing, but I'm
not
> sure it's important.

Changed.

> http://codereview.chromium.org/9639/diff/636/858
> File chrome/test/perf/perftests.scons (right):
> 
> http://codereview.chromium.org/9639/diff/636/858#newcode13
> Line 13: env.SConscript([
> Curious of SConscript vs ApplySConscript
> 
> http://codereview.chromium.org/9639/diff/636/858#newcode22
> Line 22: env.Append(
> Why the change to Append? Not that it matters, just curious.

These were extra changes, not needed. Removed.

[please use my chromium address, 2008-12-03 20:00:39]

Updated to use FilePath.Append(FilePath).

[rvargas (doing something else), 2008-12-03 21:49:43]

http://codereview.chromium.org/9639/diff/674/892
File net/disk_cache/disk_cache_perftest.cc (right):

http://codereview.chromium.org/9639/diff/674/892#newcode158
Line 158: std::wstring path_wstring = GetCachePath();
nit: Using the type for a variable name is a violation of the spirit of the
coding style.

http://codereview.chromium.org/9639/diff/674/892#newcode189
Line 189: cache = disk_cache::CreateCacheBackend(path.ToWStringHack(), false,
0);
nit: maybe we can use path_wstring directly.

[Mark Mentovai, 2008-12-03 22:07:41]

http://codereview.chromium.org/9639/diff/674/892
File net/disk_cache/disk_cache_perftest.cc (right):

http://codereview.chromium.org/9639/diff/674/892#newcode158
Line 158: std::wstring path_wstring = GetCachePath();
rvargas wrote:
> nit: Using the type for a variable name is a violation of the spirit of the
> coding style.

I think it's fine when you need to differentiate it from another variable that
holds a conversion of the same data.  In this case, |path| below is equivalent
to |path_wstring|.  The style guide means "don't name your variable age_int (or
worse, iAge)" but I don't see anything wrong with, say, deriving |GURL url| from
|std::string url_string|.

[please use my chromium address, 2008-12-03 22:12:16]

On 2008/12/03 21:49:43, rvargas wrote:
> http://codereview.chromium.org/9639/diff/674/892#newcode189
> Line 189: cache = disk_cache::CreateCacheBackend(path.ToWStringHack(), false,
> 0);
> nit: maybe we can use path_wstring directly.

One *Hack function less is a good thing. Done.

[rvargas (doing something else), 2008-12-03 23:52:46]

http://codereview.chromium.org/9639/diff/674/892
File net/disk_cache/disk_cache_perftest.cc (right):

http://codereview.chromium.org/9639/diff/674/892#newcode158
Line 158: std::wstring path_wstring = GetCachePath();
Yes, I see that, it just jumps out of the code while reading it. Maybe the
problem is having two variables to hold the same entity, but if the alternative
is to use ToWStringHack everywhere this option is much better. It was just a nit
anyway ;)


On 2008/12/03 22:07:41, Mark Mentovai wrote:
> rvargas wrote:
> > nit: Using the type for a variable name is a violation of the spirit of the
> > coding style.
> 
> I think it's fine when you need to differentiate it from another variable that
> holds a conversion of the same data.  In this case, |path| below is equivalent
> to |path_wstring|.  The style guide means "don't name your variable age_int
(or
> worse, iAge)" but I don't see anything wrong with, say, deriving |GURL url|
from
> |std::string url_string|.

[Mark Mentovai, 2008-12-05 18:06:50]

LGTM for the most part, but have you tested on Windows?

http://codereview.chromium.org/9639/diff/684/913
File chrome/browser/safe_browsing/database_perftest.cc (right):

http://codereview.chromium.org/9639/diff/684/913#newcode470
Line 470: DLOG(INFO) << path_.BaseName().value() << " read " << keys_to_read <<
That'll be a wstring on Windows, right?

[rvargas (doing something else), 2008-12-05 19:50:28]

http://codereview.chromium.org/9639/diff/684/906
File chrome/tools/perf/flush_cache/flush_cache.cc (right):

http://codereview.chromium.org/9639/diff/684/906#newcode5
Line 5: // This little program attempts to flush the disk cache for some files.
Sorry about this one, but now that you are touching this file could you change
"disk cache" for "system cache" here?. It made me go through the whole file
history to double check where does this come from. Now we have something called
disk cache that makes this comment a little misleading.

[please use my chromium address, 2008-12-08 10:01:40]

On 2008/12/05 18:06:50, Mark Mentovai wrote:
> LGTM for the most part, but have you tested on Windows?

No, but I'll try to convince someone to send it to a trybot.

> http://codereview.chromium.org/9639/diff/684/913
> File chrome/browser/safe_browsing/database_perftest.cc (right):
> 
> http://codereview.chromium.org/9639/diff/684/913#newcode470
> Line 470: DLOG(INFO) << path_.BaseName().value() << " read " << keys_to_read
<<
> That'll be a wstring on Windows, right?

Right.

rvargas: fixed.