Fix incorrect computation of LastModified in SimpleCache,

Fix incorrect computation of LastModified in SimpleCache,
avoid making useless syscalls[1]. Those particular ones seem to typically
be ultra cheap, but why do them when not needed?

The code was written to use simple_util::GetMTime because at the time it
was more precise than File::GetInfo when it came to timestamps; trouble
is, path_ is the cache directory, not the cache entry file (see e.g.
GetFilenameFromFileIndex), so this is the last modified timestamp
for the wrong thing; and as far as I can see File::GetInfo is as precise
these days.

As far as I can see w/grep and codesearch, nothing ultimately uses
DiskCache::Entry::GetLastModified() outside tests, so the biggest
impact was likely that SyncOpenEntryAge stat collected was bogus.

[1] Those were the looks up of info on the cache directory we saw when
 looking at syscall trace on Windows.

BUG=714143
Review-Url: https://codereview.chromium.org/2791493003
Cr-Commit-Position: refs/heads/master@{#466674}
Committed: https://chromium.googlesource.com/chromium/src/+/2cac5e68544bf2f5fbaec00f62ffe0c6e7957598

[Maks Orlovich, 2017-03-31 17:00:16]

The CQ bit was checked by morlovich@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-31 17:00:58]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Maks Orlovich, 2017-03-31 17:02:13]

morlovich@chromium.org changed reviewers:
+ asanka@chromium.org

[Maks Orlovich, 2017-03-31 17:02:14]

A bit worried about the test here, lots of opportunities for 
flakiness and platform-sensitivity.

[commit-bot: I haz the power, 2017-03-31 17:23:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-31 17:24:00]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[Maks Orlovich, 2017-03-31 17:36:37]

On 2017/03/31 17:02:14, Maks Orlovich wrote:
> A bit worried about the test here, lots of opportunities for 
> flakiness and platform-sensitivity.

The two failures thus far look like something up with accessibility_unittests,
though, and seem to happen to other people, too. Maybe?
I am not too great at buildbot UI.

[asanka, 2017-04-01 02:19:30]

lgtm

[Maks Orlovich, 2017-04-01 02:53:35]

The CQ bit was checked by morlovich@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-01 02:53:47]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-01 04:43:02]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-01 04:43:03]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[morlovich, 2017-04-02 13:09:57]

Now this failure is actually my fault:

DiskCacheBackendTest.SimpleLastModified (run #1):
[ RUN      ] DiskCacheBackendTest.SimpleLastModified
e:\b\c\b\win\src\net\disk_cache\backend_unittest.cc(4074): error: Expected:
(reopen_entry1->GetLastModified()) <= (entry1_timestamp), actual: 2017-04-01
04:24:18.729 UTC vs 2017-04-01 04:24:18.728 UTC
[  FAILED  ] DiskCacheBackendTest.SimpleLastModified (18 ms)

Wonder if that's a precision thing or not, will need to debug.

[Maks Orlovich, 2017-04-03 17:12:41]

On 2017/04/02 13:09:57, morlovich wrote:
> Now this failure is actually my fault:
> 
> DiskCacheBackendTest.SimpleLastModified (run #1):
> [ RUN      ] DiskCacheBackendTest.SimpleLastModified
> e:\b\c\b\win\src\net\disk_cache\backend_unittest.cc(4074): error: Expected:
> (reopen_entry1->GetLastModified()) <= (entry1_timestamp), actual: 2017-04-01
> 04:24:18.729 UTC vs 2017-04-01 04:24:18.728 UTC
> [  FAILED  ] DiskCacheBackendTest.SimpleLastModified (18 ms)
> 
> Wonder if that's a precision thing or not, will need to debug.

Looks like it's rather that I don't actually executed the first close by the
time I grab entry1_timestamp

[Maks Orlovich, 2017-04-03 19:01:26]

The CQ bit was checked by morlovich@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-03 19:02:51]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Maks Orlovich, 2017-04-03 20:34:06]

I think the test is fixed now (though I still don't really like it), PTAL.

[commit-bot: I haz the power, 2017-04-03 21:22:12]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-03 21:22:12]

Dry run: This issue passed the CQ dry run.

[Maks Orlovich, 2017-04-10 13:19:58]

Sorry, but ping on the test change?

[Maks Orlovich, 2017-04-17 14:33:28]

On 2017/04/10 13:19:58, Maks Orlovich wrote:
> Sorry, but ping on the test change?

Sorry, but ping x2, especially since you seem to be paying attention to this
now...

[jkarlin, 2017-04-21 13:48:33]

jkarlin@chromium.org changed reviewers:
+ jkarlin@chromium.org

[jkarlin, 2017-04-21 13:48:34]

drive by lgtm w/ comments

https://codereview.chromium.org/2791493003/diff/20001/net/disk_cache/backend_...
File net/disk_cache/backend_unittest.cc (right):

https://codereview.chromium.org/2791493003/diff/20001/net/disk_cache/backend_...
net/disk_cache/backend_unittest.cc:4002: TEST_F(DiskCacheBackendTest,
SimpleLastModified) {
Please add a comment describing what this test is doing and link to the crbug
it's related to

https://codereview.chromium.org/2791493003/diff/20001/net/disk_cache/backend_...
net/disk_cache/backend_unittest.cc:4027: while (base::Time::Now() <=
Also use NowFromSystemTime here?

[Maks Orlovich, 2017-04-21 14:39:40]

Description was changed from

==========
Fix incorrect computation of LastModified in SimpleCache,
avoid making useless syscalls[1]. Those particular ones seem to typically
be ultra cheap, but why do them when not needed?

The code was written to use simple_util::GetMTime because at the time it
was more precise than File::GetInfo when it came to timestamps; trouble
is, path_ is the cache directory, not the cache entry file (see e.g.
GetFilenameFromFileIndex), so this is the last modified timestamp
for the wrong thing; and as far as I can see File::GetInfo is as precise
these days.

As far as I can see w/grep and codesearch, nothing ultimately uses
DiskCache::Entry::GetLastModified() outside tests, so the biggest
impact was likely that SyncOpenEntryAge stat collected was bogus.

[1] Those were the looks up of info on the cache directory we saw when
 looking at syscall trace on Windows.

BUG=
==========

to

==========
Fix incorrect computation of LastModified in SimpleCache,
avoid making useless syscalls[1]. Those particular ones seem to typically
be ultra cheap, but why do them when not needed?

The code was written to use simple_util::GetMTime because at the time it
was more precise than File::GetInfo when it came to timestamps; trouble
is, path_ is the cache directory, not the cache entry file (see e.g.
GetFilenameFromFileIndex), so this is the last modified timestamp
for the wrong thing; and as far as I can see File::GetInfo is as precise
these days.

As far as I can see w/grep and codesearch, nothing ultimately uses
DiskCache::Entry::GetLastModified() outside tests, so the biggest
impact was likely that SyncOpenEntryAge stat collected was bogus.

[1] Those were the looks up of info on the cache directory we saw when
 looking at syscall trace on Windows.

BUG=714143
==========

[Maks Orlovich, 2017-04-21 15:10:29]

The CQ bit was checked by morlovich@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-21 15:10:44]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Maks Orlovich, 2017-04-21 15:12:52]

(Also rebased for patch3, since it wouldn't build otherwise)

https://codereview.chromium.org/2791493003/diff/20001/net/disk_cache/backend_...
File net/disk_cache/backend_unittest.cc (right):

https://codereview.chromium.org/2791493003/diff/20001/net/disk_cache/backend_...
net/disk_cache/backend_unittest.cc:4002: TEST_F(DiskCacheBackendTest,
SimpleLastModified) {
On 2017/04/21 13:48:33, jkarlin wrote:
> Please add a comment describing what this test is doing and link to the crbug
> it's related to

Done.

https://codereview.chromium.org/2791493003/diff/20001/net/disk_cache/backend_...
net/disk_cache/backend_unittest.cc:4027: while (base::Time::Now() <=
On 2017/04/21 13:48:33, jkarlin wrote:
> Also use NowFromSystemTime here?

Done.

[jkarlin, 2017-04-21 16:39:07]

slgtm

[commit-bot: I haz the power, 2017-04-21 16:41:59]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-21 16:42:00]

Dry run: This issue passed the CQ dry run.

[asanka, 2017-04-21 19:23:50]

lgtm

[Maks Orlovich, 2017-04-24 13:03:20]

The CQ bit was checked by morlovich@chromium.org

[Maks Orlovich, 2017-04-24 13:03:20]

The patchset sent to the CQ was uploaded after l-g-t-m from jkarlin@chromium.org
Link to the patchset: https://codereview.chromium.org/2791493003/#ps40001
(title: "Apply review feedback")

[commit-bot: I haz the power, 2017-04-24 13:03:46]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-24 15:03:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-24 15:03:31]

Try jobs failed on following builders:
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[Maks Orlovich, 2017-04-24 15:18:38]

On 2017/04/24 15:03:31, commit-bot: I haz the power wrote:
> Try jobs failed on following builders:
>   win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
>
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

Failure in test_installer
I am guessing probably unrelated, so will retry..

[Maks Orlovich, 2017-04-24 15:18:54]

The CQ bit was checked by morlovich@chromium.org

[commit-bot: I haz the power, 2017-04-24 15:19:24]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-24 17:40:02]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1493047134151240,
"parent_rev": "177d44c2b478ca96d5cff47940509b578e4af697", "commit_rev":
"4949ed4d4b6c6cb60accd9b33740a26c9990c837"}

[commit-bot: I haz the power, 2017-04-24 17:40:22]

CQ is committing da patch.

Bot data: {"patchset_id": 40001, "attempt_start_ts": 1493047134151240,
"parent_rev": "7e94ab42060d523aec73298bcc793eb0561259cb", "commit_rev":
"2cac5e68544bf2f5fbaec00f62ffe0c6e7957598"}

[commit-bot: I haz the power, 2017-04-24 17:40:33]

Description was changed from

==========
Fix incorrect computation of LastModified in SimpleCache,
avoid making useless syscalls[1]. Those particular ones seem to typically
be ultra cheap, but why do them when not needed?

The code was written to use simple_util::GetMTime because at the time it
was more precise than File::GetInfo when it came to timestamps; trouble
is, path_ is the cache directory, not the cache entry file (see e.g.
GetFilenameFromFileIndex), so this is the last modified timestamp
for the wrong thing; and as far as I can see File::GetInfo is as precise
these days.

As far as I can see w/grep and codesearch, nothing ultimately uses
DiskCache::Entry::GetLastModified() outside tests, so the biggest
impact was likely that SyncOpenEntryAge stat collected was bogus.

[1] Those were the looks up of info on the cache directory we saw when
 looking at syscall trace on Windows.

BUG=714143
==========

to

==========
Fix incorrect computation of LastModified in SimpleCache,
avoid making useless syscalls[1]. Those particular ones seem to typically
be ultra cheap, but why do them when not needed?

The code was written to use simple_util::GetMTime because at the time it
was more precise than File::GetInfo when it came to timestamps; trouble
is, path_ is the cache directory, not the cache entry file (see e.g.
GetFilenameFromFileIndex), so this is the last modified timestamp
for the wrong thing; and as far as I can see File::GetInfo is as precise
these days.

As far as I can see w/grep and codesearch, nothing ultimately uses
DiskCache::Entry::GetLastModified() outside tests, so the biggest
impact was likely that SyncOpenEntryAge stat collected was bogus.

[1] Those were the looks up of info on the cache directory we saw when
 looking at syscall trace on Windows.

BUG=714143

Review-Url: https://codereview.chromium.org/2791493003
Cr-Commit-Position: refs/heads/master@{#466674}
Committed:
https://chromium.googlesource.com/chromium/src/+/2cac5e68544bf2f5fbaec00f62ff...
==========

[commit-bot: I haz the power, 2017-04-24 17:40:34]

Committed patchset #3 (id:40001) as
https://chromium.googlesource.com/chromium/src/+/2cac5e68544bf2f5fbaec00f62ff...