Simple Cache: avoid extraneous stream 0 (http header) writes.

Simple Cache: avoid extraneous stream 0 (http header) writes.

These writes were not only wasteful, but they could cause corruption
if the existing EOF record didn't have a key SHA256; the new key
SHA256 would overwrite the existing one corrupting the entry.

BUG=615440
R=juliatuttle@chromium.org,rdsmith@chromium.org

Committed: https://crrev.com/6b66971b761a15a67982bef692619a96f306150d
Cr-Commit-Position: refs/heads/master@{#396533}

[gavinp, 2016-05-27 17:04:30]

juliatuttle: PTAL. This will need to be merged into m52 as well.

rdsmith: FYI.

[Randy Smith (Not in Mondays), 2016-05-27 17:35:52]

So this is a behavior change, from unilaterally writing the HTTP headers to only
writing them if the CRC needs to be written?  Is that what you want?  I'm
worried in the abstract about a behavior change when it isn't needed--I don't
think we're relying on it anywhere else, but one possible reliance that I had to
check the code to confirm wasn't an issue was whether we wrote the CRC when *it*
changed or when the data it referred to change (it's the latter, IIUC).  But
that gives you the flavor of my concerns about the behavior change.

Separately: Ooops, I'm sorry I didn't catch this in the original review.  The
virtues of having multiple people actively working on a single project is that
more than one person will have this kind of thing active in their head.  Long
term (not this CL) I'd be in favor of refactoring the read and write paths so
that the calls to, e.g., WriteEOFRecord(stream=0), WriteSHA256, WriteCRC were
single function calls and it was dead easy to scan the conditionalization for
when they happened and when they didn't.

[Randy Smith (Not in Mondays), 2016-05-27 17:40:04]

On 2016/05/27 17:35:52, Randy Smith - Not in Fridays wrote:
> So this is a behavior change, from unilaterally writing the HTTP headers to
only
> writing them if the CRC needs to be written?  Is that what you want?  I'm
> worried in the abstract about a behavior change when it isn't needed--I don't
> think we're relying on it anywhere else, but one possible reliance that I had
to
> check the code to confirm wasn't an issue was whether we wrote the CRC when
*it*
> changed or when the data it referred to change (it's the latter, IIUC).  But
> that gives you the flavor of my concerns about the behavior change.
> 
> Separately: Ooops, I'm sorry I didn't catch this in the original review.  The
> virtues of having multiple people actively working on a single project is that
> more than one person will have this kind of thing active in their head.  Long
> term (not this CL) I'd be in favor of refactoring the read and write paths so
> that the calls to, e.g., WriteEOFRecord(stream=0), WriteSHA256, WriteCRC were
> single function calls and it was dead easy to scan the conditionalization for
> when they happened and when they didn't.

Suggestion: Leave the stream 0 write where it is, but tie the SHA256 write to
the stream 0 CRC entry write, so it doesn't happen unless the CRC write happens?

The rest of the CL looks good; I just want to nail down whether this is a
behavior change, and whether it's necessary if so.

[Randy Smith (Not in Mondays), 2016-05-27 17:47:12]

Offline conversation (and more closely reading the CL description :-}) confirms
the behavior change is intentional, and I agree it's a good thing.  LGTM.

[gavinp, 2016-05-27 17:47:42]

On 2016/05/27 17:40:04, Randy Smith - Not in Fridays wrote:
> On 2016/05/27 17:35:52, Randy Smith - Not in Fridays wrote:
> > So this is a behavior change, from unilaterally writing the HTTP headers to
> only
> > writing them if the CRC needs to be written?  Is that what you want?  I'm
> > worried in the abstract about a behavior change when it isn't needed--I
don't
> > think we're relying on it anywhere else, but one possible reliance that I
had
> to
> > check the code to confirm wasn't an issue was whether we wrote the CRC when
> *it*
> > changed or when the data it referred to change (it's the latter, IIUC).  But
> > that gives you the flavor of my concerns about the behavior change.
> > 
> > Separately: Ooops, I'm sorry I didn't catch this in the original review. 
The
> > virtues of having multiple people actively working on a single project is
that
> > more than one person will have this kind of thing active in their head. 
Long
> > term (not this CL) I'd be in favor of refactoring the read and write paths
so
> > that the calls to, e.g., WriteEOFRecord(stream=0), WriteSHA256, WriteCRC
were
> > single function calls and it was dead easy to scan the conditionalization
for
> > when they happened and when they didn't.
> 
> Suggestion: Leave the stream 0 write where it is, but tie the SHA256 write to
> the stream 0 CRC entry write, so it doesn't happen unless the CRC write
happens?
> 
> The rest of the CL looks good; I just want to nail down whether this is a
> behavior change, and whether it's necessary if so.

I think it's a desirable change, the header write isn't necessary unless there's
a CRC write; going through simple entry impl we are careful about invalidating
stream 0 when stream 1 is mutated, which forces the header write and crc write,
which is correct, since the headers have moved.

In an earlier revision of this CL I had a test that opened an entry, wrote
stream 1, then resized stream 1, and confirmed the entry could still be opened
(i.e. did stream 0 data get written and an EOF record written?)

[gavinp, 2016-05-27 17:48:46]

The CQ bit was checked by gavinp@chromium.org

[commit-bot: I haz the power, 2016-05-27 17:49:04]

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

[commit-bot: I haz the power, 2016-05-27 19:33:16]

Committed patchset #1 (id:1)

[commit-bot: I haz the power, 2016-05-27 19:34:21]

Description was changed from

==========
Simple Cache: avoid extraneous stream 0 (http header) writes.

These writes were not only wasteful, but they could cause corruption
if the existing EOF record didn't have a key SHA256; the new key
SHA256 would overwrite the existing one corrupting the entry.

BUG=615440
R=juliatuttle@chromium.org,rdsmith@chromium.org
==========

to

==========
Simple Cache: avoid extraneous stream 0 (http header) writes.

These writes were not only wasteful, but they could cause corruption
if the existing EOF record didn't have a key SHA256; the new key
SHA256 would overwrite the existing one corrupting the entry.

BUG=615440
R=juliatuttle@chromium.org,rdsmith@chromium.org

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

[commit-bot: I haz the power, 2016-05-27 19:34:22]

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