Fix loop early exit bug in blockfile cache Trim behavior.

Fix loop early exit bug in blockfile cache Trim behavior.

This bug fixes an issue in which a "done" bool in a loop is never set
since it is reinitialized at the top of the loop body.

(Detected as a dead store by the Clang static analyzer.)

R=wez@chromium.org,msramek@chromium.org
BUG=686838
Review-Url: https://codereview.chromium.org/2673863003
Cr-Commit-Position: refs/heads/master@{#449392}
Committed: https://chromium.googlesource.com/chromium/src/+/468141a302bc1a593c5f9ec5286e858cf3a5f465

[Kevin M, 2017-02-04 01:16:33]

Description was changed from

==========
Fix loop early exit bug in disk cache Trim bheavior.

This bug fixes an issue in which a "done" bool in a loop is never set
since it is reinitialized at the top of the loop body.

(Detected as a dead store by the Clang static analyzer.)

R=wez@chromium.org,msramek@chromium.org
BUG=686838
==========

to

==========
Fix loop early exit bug in disk cache Trim bheavior.

This bug fixes an issue in which a "done" bool in a loop is never set
since it is reinitialized at the top of the loop body.

(Detected as a dead store by the Clang static analyzer.)

R=wez@chromium.org,msramek@chromium.org
BUG=686838
==========

[Kevin M, 2017-02-04 01:17:13]

Description was changed from

==========
Fix loop early exit bug in disk cache Trim bheavior.

This bug fixes an issue in which a "done" bool in a loop is never set
since it is reinitialized at the top of the loop body.

(Detected as a dead store by the Clang static analyzer.)

R=wez@chromium.org,msramek@chromium.org
BUG=686838
==========

to

==========
Fix loop early exit bug in blockfile cache Trim behavior.

This bug fixes an issue in which a "done" bool in a loop is never set
since it is reinitialized at the top of the loop body.

(Detected as a dead store by the Clang static analyzer.)

R=wez@chromium.org,msramek@chromium.org
BUG=686838
==========

[Kevin M, 2017-02-04 01:17:47]

Not a high priority issue, but it popped up during a spot inspection of the
Clang analyzer results.

[Wez, 2017-02-04 08:08:02]

LGTM

Looks like the effect of this is to alter which cache entry gets evicted?

[msramek, 2017-02-06 09:44:21]

Wow, this code is from 2009. By now, it could easily be expected behavior not to
quit the loop :)

LGTM, but I'm not an owner.

[msramek, 2017-02-06 09:44:33]

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

[commit-bot: I haz the power, 2017-02-06 09:44:48]

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-02-06 10:44:41]

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

[commit-bot: I haz the power, 2017-02-06 10:44:42]

Dry run: This issue passed the CQ dry run.

[Kevin M, 2017-02-06 17:56:38]

The CQ bit was checked by kmarshall@chromium.org

[commit-bot: I haz the power, 2017-02-06 17:57:07]

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-02-06 18:04:43]

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

[commit-bot: I haz the power, 2017-02-06 18:04:44]

Try jobs failed on following builders:
  chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)

[Kevin M, 2017-02-06 18:10:33]

kmarshall@chromium.org changed reviewers:
+ mmenke@chromium.org

[Kevin M, 2017-02-06 18:10:33]

mmenke for OWNERS review

[Kevin M, 2017-02-06 18:34:02]

kmarshall@chromium.org changed reviewers:
+ cbentzel@chromium.org
- mmenke@chromium.org

[Kevin M, 2017-02-06 18:34:02]

R += cbentzel (mmenke is OOO)

[cbentzel, 2017-02-08 23:27:16]

On 2017/02/06 18:34:02, Kevin M wrote:
> R += cbentzel (mmenke is OOO)

Sorry for delay. This does LGTM, I was trying to understand testing and what
possibe uninted side-effects would be. But still LGTM.

[Wez, 2017-02-09 00:47:37]

On 2017/02/08 23:27:16, cbentzel wrote:
> On 2017/02/06 18:34:02, Kevin M wrote:
> > R += cbentzel (mmenke is OOO)
> 
> Sorry for delay. This does LGTM, I was trying to understand testing and what
> possibe uninted side-effects would be. But still LGTM.

IIUC the broken version of the code will incorrectly prioritize eviction of
HIGH_USE nodes, rather than unused or LOW_USE ones, which sounds like it may
actually have significant run-time performance impact?

[Kevin M, 2017-02-09 19:06:13]

The CQ bit was checked by kmarshall@chromium.org

[commit-bot: I haz the power, 2017-02-09 19:07:05]

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-02-09 20:24:42]

CQ is committing da patch.

Bot data: {"patchset_id": 1, "attempt_start_ts": 1486667173820240, "parent_rev":
"76115ff06c69c188db4f51db7816cb5700d63788", "commit_rev":
"468141a302bc1a593c5f9ec5286e858cf3a5f465"}

[commit-bot: I haz the power, 2017-02-09 20:25:12]

Description was changed from

==========
Fix loop early exit bug in blockfile cache Trim behavior.

This bug fixes an issue in which a "done" bool in a loop is never set
since it is reinitialized at the top of the loop body.

(Detected as a dead store by the Clang static analyzer.)

R=wez@chromium.org,msramek@chromium.org
BUG=686838
==========

to

==========
Fix loop early exit bug in blockfile cache Trim behavior.

This bug fixes an issue in which a "done" bool in a loop is never set
since it is reinitialized at the top of the loop body.

(Detected as a dead store by the Clang static analyzer.)

R=wez@chromium.org,msramek@chromium.org
BUG=686838

Review-Url: https://codereview.chromium.org/2673863003
Cr-Commit-Position: refs/heads/master@{#449392}
Committed:
https://chromium.googlesource.com/chromium/src/+/468141a302bc1a593c5f9ec5286e...
==========

[commit-bot: I haz the power, 2017-02-09 20:25:13]

Committed patchset #1 (id:1) as
https://chromium.googlesource.com/chromium/src/+/468141a302bc1a593c5f9ec5286e...