Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1977863003:
SimpleCache: open with fewer seeks. (Closed)
|
Created:
4 years, 7 months ago by gavinp Modified:
4 years, 7 months ago CC:
chromium-reviews, cbentzel+watch_chromium.org, gavinp+disk_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@open-speeder-macbetter Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionSimpleCache: open with fewer seeks.
There's usually no reason to read the file header before reading the
stream 0 (headers) data when opening a simple cache entry. By not doing
so, seeks in the critical path of the loading headers are reduced, which
should speed up http caching performance.
To be careful, the headers are still checked when the first read of data
(if any) is performed. This delays detecting hash collisions in the
cache, because the header data is required to compare keys (instead of
just hashes).
To compensate for the later key comparison, a SHA256 of the key is
stored as an optional prefix to the end of file record. This is used to
validate the key is correct before returning headers to the client.
Because the key checking logic is now totally migrated back into the
SimpleSynchronousEntry, we can return to reporting on key matching in
the SyncOpenResult histograms.
R=juliatuttle@chromium.org,asvitkine@chromium.org,rdsmith@chromium.org
BUG=611732
Committed: https://crrev.com/9f15f2258d7cae8cd12cb4ef7143ad648a958333
Cr-Commit-Position: refs/heads/master@{#395083}
Patch Set 1 #Patch Set 2 : cleaner, check header eventually #Patch Set 3 : cleaner still #Patch Set 4 : self reviewed #
Total comments: 2
Patch Set 5 : move obsolete section #
Total comments: 26
Patch Set 6 : add unit test #Patch Set 7 : remediate #
Total comments: 5
Patch Set 8 : second round remediation #Patch Set 9 : eliminate dead conditional #Patch Set 10 : moar remediation #Patch Set 11 : add a key SHA256 check #
Total comments: 4
Patch Set 12 : better histograms #Patch Set 13 : fix histograms.xml #Patch Set 14 : histogram suffixes #
Total comments: 10
Patch Set 15 : add unit tests #
Total comments: 8
Patch Set 16 : remediate to review #Patch Set 17 : speeeling #Patch Set 18 : fix leak in unittest #Patch Set 19 : fix entry corruption test #Patch Set 20 : actually fix test #Messages
Total messages: 48 (17 generated)
Looks promising: MacOS shows a 30% improvement on cold reads of headers, Linux shows a 20% improvement.
Description was changed from ========== **WIP** SimpleCache: open with fewer seeks. Let's be much more parsimonious with calls to File::Read(), esp those that are likely causing seeks. R= BUG=611732 ========== to ========== SimpleCache: open with fewer seeks. There's usually no reason to read the file header before reading the index data when opening a simple cache entry. By not doing so, seeks in the critical path of the headers are reduced, which should speed up http caching performance. To be careful, the headers are still checked when the first read of data (if any) is performed. Because the key checking logic is now totally migrated back into the SimpleSynchronousEntry, we can return to reporting on key matching in the SyncOpenResult histograms. R=juliatuttle@chromium.org,asvitkine@chromium.org BUG=611732 ==========
gavinp@chromium.org changed reviewers: + asvitkine@chromium.org, juliatuttle@chromium.org
ptal
histograms lgtm https://codereview.chromium.org/1977863003/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1977863003/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:50479: + <obsolete> Nit: I think this goes before the owner?
Thanks for the quick review Alexei https://codereview.chromium.org/1977863003/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1977863003/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:50479: + <obsolete> On 2016/05/16 16:47:13, Alexei Svitkine wrote: > Nit: I think this goes before the owner?
https://codereview.chromium.org/1977863003/diff/80001/net/disk_cache/simple/s... File net/disk_cache/simple/simple_synchronous_entry.cc (right): https://codereview.chromium.org/1977863003/diff/80001/net/disk_cache/simple/s... net/disk_cache/simple/simple_synchronous_entry.cc:959: size_t expected_header_size = GetHeaderSize(header->key_length); I think this needs a unit test; just something with a key size that's large enough to trigger this read, and confirmation that it can be iterated. https://codereview.chromium.org/1977863003/diff/80001/net/disk_cache/simple/s... net/disk_cache/simple/simple_synchronous_entry.cc:1013: "simple-cache-dont-read-initial-header")) { Oh, yes, this must be removed for landing. I was using this for testing the impact of this change using the disk cache perftool.
Description was changed from ========== SimpleCache: open with fewer seeks. There's usually no reason to read the file header before reading the index data when opening a simple cache entry. By not doing so, seeks in the critical path of the headers are reduced, which should speed up http caching performance. To be careful, the headers are still checked when the first read of data (if any) is performed. Because the key checking logic is now totally migrated back into the SimpleSynchronousEntry, we can return to reporting on key matching in the SyncOpenResult histograms. R=juliatuttle@chromium.org,asvitkine@chromium.org BUG=611732 ========== to ========== SimpleCache: open with fewer seeks. There's usually no reason to read the file header before reading the index data when opening a simple cache entry. By not doing so, seeks in the critical path of the headers are reduced, which should speed up http caching performance. To be careful, the headers are still checked when the first read of data (if any) is performed. Because the key checking logic is now totally migrated back into the SimpleSynchronousEntry, we can return to reporting on key matching in the SyncOpenResult histograms. R=juliatuttle@chromium.org,asvitkine@chromium.org,rdsmith@chromium.org BUG=611732 ==========
gavinp@chromium.org changed reviewers: + rdsmith@chromium.org
rdsmith: I think juliatuttle is out. Can you PTAL?
This looks like goodness, though admittedly subtle goodness :-}. I haven't reviewed the tests yet (I think you're still working on them?) but here are my initial comments. You and I agreed offline that you'd update the CL description in two ways: * Make clearer what you meant by "index data" (I think you actually meant "Stream 0"?) * Call out the new danger around hash collisions and revalidation, since we're no longer checking for collisions before revalidation. https://codereview.chromium.org/1977863003/diff/80001/net/disk_cache/simple/s... File net/disk_cache/simple/simple_entry_impl.cc (left): https://codereview.chromium.org/1977863003/diff/80001/net/disk_cache/simple/s... net/disk_cache/simple/simple_entry_impl.cc:1150: if (key_.empty()) { It's not really part of this CL, but I'd value having more documentation on SimpleEntryImpl about when it can or can't have a key_ and how its behavior changes in those cases. Up to you whether to do that while you're here, but it's not obvious reading the code that key_ (as set through SetKey()) is an optional useful addition to the SEI). https://codereview.chromium.org/1977863003/diff/80001/net/disk_cache/simple/s... File net/disk_cache/simple/simple_synchronous_entry.cc (right): https://codereview.chromium.org/1977863003/diff/80001/net/disk_cache/simple/s... net/disk_cache/simple/simple_synchronous_entry.cc:935: // mean an extra memory to memory copy. Hmmm. The other issue I'd add to this comment (presuming you agree) is the waste if kInitialHeaderRead actually reads into the entry; then it's not just the two separate reads, but the two reads doing the same thing. I presume that won't happen often because you'll usually have the key_ and GetHeaderSize() is mostly reliable? https://codereview.chromium.org/1977863003/diff/80001/net/disk_cache/simple/s... net/disk_cache/simple/simple_synchronous_entry.cc:947: header = reinterpret_cast<const SimpleFileHeader*>(header_data.data()); Maybe comment here about dodging the resize() repositioning chainsaw? https://codereview.chromium.org/1977863003/diff/80001/net/disk_cache/simple/s... net/disk_cache/simple/simple_synchronous_entry.cc:959: size_t expected_header_size = GetHeaderSize(header->key_length); On 2016/05/16 23:19:28, gavinp wrote: > I think this needs a unit test; just something with a key size that's large > enough to trigger this read, and confirmation that it can be iterated. Agreed. https://codereview.chromium.org/1977863003/diff/80001/net/disk_cache/simple/s... net/disk_cache/simple/simple_synchronous_entry.cc:967: header_data.resize(old_size); Call out that this is an error path? https://codereview.chromium.org/1977863003/diff/80001/net/disk_cache/simple/s... net/disk_cache/simple/simple_synchronous_entry.cc:1013: "simple-cache-dont-read-initial-header")) { On 2016/05/16 23:19:28, gavinp wrote: > Oh, yes, this must be removed for landing. I was using this for testing the > impact of this change using the disk cache perftool. Acknowledged. https://codereview.chromium.org/1977863003/diff/80001/net/disk_cache/simple/s... File net/disk_cache/simple/simple_synchronous_entry.h (right): https://codereview.chromium.org/1977863003/diff/80001/net/disk_cache/simple/s... net/disk_cache/simple/simple_synchronous_entry.h:117: // |had_index| is provided only for histograms. I'm torn here, because for someone reading the code, knowing that the key parameter will exist except for the iteration case is really really useful, but it's not appropriate to document at this level. Maybe document in the CL description and/or the first location upstack from this call that has knowledge of the calling API? https://codereview.chromium.org/1977863003/diff/80001/net/disk_cache/simple/s... net/disk_cache/simple/simple_synchronous_entry.h:333: // validate them before completing. Suggestion: I'd find this comment more understandable in the context of the code if it said ".. when an entry's headers are not checked at time of open ...". The phrasing above implies to me that they might have been checked at some point before open, which I don't think is true or makes sense :-}. https://codereview.chromium.org/1977863003/diff/80001/net/disk_cache/simple/s... File net/disk_cache/simple/simple_util.cc (right): https://codereview.chromium.org/1977863003/diff/80001/net/disk_cache/simple/s... net/disk_cache/simple/simple_util.cc:104: int32_t GetDataSizeFromFileSize(size_t key_length, int64_t file_size) { Suggestion: So I know that the definition is changing, but the old name still seems right to me; it just parses differently (GetDataSizeFrom(KeyAndFile)Size). It feels weird to imply that the function is ignoring the key size (also below). (Suggestion because I don't care a lot.)
Description was changed from ========== SimpleCache: open with fewer seeks. There's usually no reason to read the file header before reading the index data when opening a simple cache entry. By not doing so, seeks in the critical path of the headers are reduced, which should speed up http caching performance. To be careful, the headers are still checked when the first read of data (if any) is performed. Because the key checking logic is now totally migrated back into the SimpleSynchronousEntry, we can return to reporting on key matching in the SyncOpenResult histograms. R=juliatuttle@chromium.org,asvitkine@chromium.org,rdsmith@chromium.org BUG=611732 ========== to ========== SimpleCache: open with fewer seeks. There's usually no reason to read the file header before reading the stream 0 (headers) data when opening a simple cache entry. By not doing so, seeks in the critical path of the loading headers are reduced, which should speed up http caching performance. To be careful, the headers are still checked when the first read of data (if any) is performed. This delays detecting hash collisions in the cache, because the header data is required to compare keys (instead of just hashes). A hash collision is still detected before the first read of the data from the entry. But this still means that http requests can fail if there's hash collisions. If there's an http request for a resource, and the cache contains a collision, then the cache will give the wrong headers to the HTTP Cache Transaction. If it then issues a conditional request which 304s, the request will fail on the first read. Because the key checking logic is now totally migrated back into the SimpleSynchronousEntry, we can return to reporting on key matching in the SyncOpenResult histograms. R=juliatuttle@chromium.org,asvitkine@chromium.org,rdsmith@chromium.org BUG=611732 ==========
Randy, Thanks for your review; I've now addressed your comments. As well, there's now a test. https://codereview.chromium.org/1977863003/diff/80001/net/disk_cache/simple/s... File net/disk_cache/simple/simple_entry_impl.cc (left): https://codereview.chromium.org/1977863003/diff/80001/net/disk_cache/simple/s... net/disk_cache/simple/simple_entry_impl.cc:1150: if (key_.empty()) { On 2016/05/17 20:04:07, Randy Smith - Not in Fridays wrote: > It's not really part of this CL, but I'd value having more documentation on > SimpleEntryImpl about when it can or can't have a key_ and how its behavior > changes in those cases. Up to you whether to do that while you're here, but > it's not obvious reading the code that key_ (as set through SetKey()) is an > optional useful addition to the SEI). Done. https://codereview.chromium.org/1977863003/diff/80001/net/disk_cache/simple/s... File net/disk_cache/simple/simple_synchronous_entry.cc (right): https://codereview.chromium.org/1977863003/diff/80001/net/disk_cache/simple/s... net/disk_cache/simple/simple_synchronous_entry.cc:935: // mean an extra memory to memory copy. On 2016/05/17 20:04:07, Randy Smith - Not in Fridays wrote: > Hmmm. The other issue I'd add to this comment (presuming you agree) is the > waste if kInitialHeaderRead actually reads into the entry; then it's not just > the two separate reads, but the two reads doing the same thing. I presume that > won't happen often because you'll usually have the key_ and GetHeaderSize() is > mostly reliable? Yeah, the key is almost always present. And in iteration, it's unlikely that the entry will subsequently be read. More typically, it's doomed. Called it out. https://codereview.chromium.org/1977863003/diff/80001/net/disk_cache/simple/s... net/disk_cache/simple/simple_synchronous_entry.cc:947: header = reinterpret_cast<const SimpleFileHeader*>(header_data.data()); On 2016/05/17 20:04:07, Randy Smith - Not in Fridays wrote: > Maybe comment here about dodging the resize() repositioning chainsaw? Actually, I checked, and resizes down in size (which this is) does not adjust capacity(), so there's no iterator invalidation. https://codereview.chromium.org/1977863003/diff/80001/net/disk_cache/simple/s... net/disk_cache/simple/simple_synchronous_entry.cc:959: size_t expected_header_size = GetHeaderSize(header->key_length); On 2016/05/17 20:04:07, Randy Smith - Not in Fridays wrote: > On 2016/05/16 23:19:28, gavinp wrote: > > I think this needs a unit test; just something with a key size that's large > > enough to trigger this read, and confirmation that it can be iterated. > > Agreed. Done. https://codereview.chromium.org/1977863003/diff/80001/net/disk_cache/simple/s... net/disk_cache/simple/simple_synchronous_entry.cc:967: header_data.resize(old_size); On 2016/05/17 20:04:07, Randy Smith - Not in Fridays wrote: > Call out that this is an error path? Done. https://codereview.chromium.org/1977863003/diff/80001/net/disk_cache/simple/s... File net/disk_cache/simple/simple_synchronous_entry.h (right): https://codereview.chromium.org/1977863003/diff/80001/net/disk_cache/simple/s... net/disk_cache/simple/simple_synchronous_entry.h:117: // |had_index| is provided only for histograms. On 2016/05/17 20:04:07, Randy Smith - Not in Fridays wrote: > I'm torn here, because for someone reading the code, knowing that the key > parameter will exist except for the iteration case is really really useful, but > it's not appropriate to document at this level. Maybe document in the CL > description and/or the first location upstack from this call that has knowledge > of the calling API? Done. Documented it at SetKey in SimpleEntryImpl. https://codereview.chromium.org/1977863003/diff/80001/net/disk_cache/simple/s... net/disk_cache/simple/simple_synchronous_entry.h:333: // validate them before completing. On 2016/05/17 20:04:08, Randy Smith - Not in Fridays wrote: > Suggestion: I'd find this comment more understandable in the context of the code > if it said ".. when an entry's headers are not checked at time of open ...". > The phrasing above implies to me that they might have been checked at some point > before open, which I don't think is true or makes sense :-}. Done. https://codereview.chromium.org/1977863003/diff/80001/net/disk_cache/simple/s... File net/disk_cache/simple/simple_util.cc (right): https://codereview.chromium.org/1977863003/diff/80001/net/disk_cache/simple/s... net/disk_cache/simple/simple_util.cc:104: int32_t GetDataSizeFromFileSize(size_t key_length, int64_t file_size) { On 2016/05/17 20:04:08, Randy Smith - Not in Fridays wrote: > Suggestion: So I know that the definition is changing, but the old name still > seems right to me; it just parses differently (GetDataSizeFrom(KeyAndFile)Size). > It feels weird to imply that the function is ignoring the key size (also > below). > > (Suggestion because I don't care a lot.) Acknowledged.
Ok, at a detailed level I'm good with this CL, with the nits below. I'd like a test that covers the "stream 0 read succeeds, stream 1 fails" case, even if it isn't because of a hash collision; can you do that by creating an entry in which the key and hash don't match? It looks like that isn't checked until CheckHeaderAndKeyForOpen. However, I'm still chewing on the whole "hash collision can result in incorrect headers being returned for a cache read" issue. I'm ok with that theoretically with very low probability resulting in a request failure if the headers say to revalidate, the source returns a 304, and we say "Whoops, we don't have it after all". But I want to make sure that's our only failure mode, and I may pull in some other eyes for that reason. Specific questions in that space: * Could you include a summary of your evaluation of the probabilities of problems (both random and malicious) in the CL description? * Does failure in the above validate->read case doom the entry? * Would it make sense (not asking for it, just curious what you think) to include a cross-hash (different function) of the key in the EOF records? We should probably talk about this verbally. https://codereview.chromium.org/1977863003/diff/80001/net/disk_cache/simple/s... File net/disk_cache/simple/simple_synchronous_entry.cc (right): https://codereview.chromium.org/1977863003/diff/80001/net/disk_cache/simple/s... net/disk_cache/simple/simple_synchronous_entry.cc:931: bool SimpleSynchronousEntry::CheckHeaderAndKeyForOpen(int file_index) { Given that this function is no longer being called during open, maybe a different name? Maybe just deleting "For Open"? https://codereview.chromium.org/1977863003/diff/80001/net/disk_cache/simple/s... net/disk_cache/simple/simple_synchronous_entry.cc:935: // mean an extra memory to memory copy. On 2016/05/18 15:46:00, gavinp wrote: > On 2016/05/17 20:04:07, Randy Smith - Not in Fridays wrote: > > Hmmm. The other issue I'd add to this comment (presuming you agree) is the > > waste if kInitialHeaderRead actually reads into the entry; then it's not just > > the two separate reads, but the two reads doing the same thing. I presume > that > > won't happen often because you'll usually have the key_ and GetHeaderSize() is > > mostly reliable? > > Yeah, the key is almost always present. And in iteration, it's unlikely that the > entry will subsequently be read. More typically, it's doomed. > > Called it out. Ah, good point; I hadn't kept in mind no key -> iteration -> probably just deleting based on time. You're welcome to go back to the old comment or not as you choose. https://codereview.chromium.org/1977863003/diff/80001/net/disk_cache/simple/s... net/disk_cache/simple/simple_synchronous_entry.cc:970: // The resize calls invalidated |*header|, so it must be reset. Why delete the comment? This was an expansion, so it may invalidate iterators. https://codereview.chromium.org/1977863003/diff/80001/net/disk_cache/simple/s... net/disk_cache/simple/simple_synchronous_entry.cc:1013: "simple-cache-dont-read-initial-header")) { On 2016/05/17 20:04:07, Randy Smith - Not in Fridays wrote: > On 2016/05/16 23:19:28, gavinp wrote: > > Oh, yes, this must be removed for landing. I was using this for testing the > > impact of this change using the disk cache perftool. > > Acknowledged. I'd feel better about it being gone in the PS I stamp? What's blocking its removal? https://codereview.chromium.org/1977863003/diff/120001/net/disk_cache/simple/... File net/disk_cache/simple/simple_entry_impl.cc (left): https://codereview.chromium.org/1977863003/diff/120001/net/disk_cache/simple/... net/disk_cache/simple/simple_entry_impl.cc:1156: } Why eliminate the DCHECK? https://codereview.chromium.org/1977863003/diff/120001/net/disk_cache/simple/... File net/disk_cache/simple/simple_synchronous_entry.cc (right): https://codereview.chromium.org/1977863003/diff/120001/net/disk_cache/simple/... net/disk_cache/simple/simple_synchronous_entry.cc:982: if (header_data.size() < sizeof(*header) + header->key_length) { Why do it by hand here and use GetHeaderSize() above? https://codereview.chromium.org/1977863003/diff/120001/net/disk_cache/simple/... net/disk_cache/simple/simple_synchronous_entry.cc:982: if (header_data.size() < sizeof(*header) + header->key_length) { Suggestion: If I'm reading the code correctly, this if statement could be pulled into the conditional of the above if statement (if the header_data wasn't too small then, it won't be now either), which would allow you to return without having to do the resize(). I think it would be a minor improvement in code readability, even given the nested conditionals. But up to you.
Description was changed from ========== SimpleCache: open with fewer seeks. There's usually no reason to read the file header before reading the stream 0 (headers) data when opening a simple cache entry. By not doing so, seeks in the critical path of the loading headers are reduced, which should speed up http caching performance. To be careful, the headers are still checked when the first read of data (if any) is performed. This delays detecting hash collisions in the cache, because the header data is required to compare keys (instead of just hashes). A hash collision is still detected before the first read of the data from the entry. But this still means that http requests can fail if there's hash collisions. If there's an http request for a resource, and the cache contains a collision, then the cache will give the wrong headers to the HTTP Cache Transaction. If it then issues a conditional request which 304s, the request will fail on the first read. Because the key checking logic is now totally migrated back into the SimpleSynchronousEntry, we can return to reporting on key matching in the SyncOpenResult histograms. R=juliatuttle@chromium.org,asvitkine@chromium.org,rdsmith@chromium.org BUG=611732 ========== to ========== SimpleCache: open with fewer seeks. There's usually no reason to read the file header before reading the stream 0 (headers) data when opening a simple cache entry. By not doing so, seeks in the critical path of the loading headers are reduced, which should speed up http caching performance. To be careful, the headers are still checked when the first read of data (if any) is performed. This delays detecting hash collisions in the cache, because the header data is required to compare keys (instead of just hashes). A hash collision is still detected before the first read of the data from the entry. But this still means that http requests can fail if there's hash collisions. If there's an http request for a resource, and the cache contains a collision, then the cache will give the wrong headers to the HTTP Cache Transaction. If it then issues a conditional request which 304s, the request will fail on the first read. The hashes are 64 bit substrings of the SHA1 of the key. To put the risk of collision in perspective, consider a cache with 23,000 entries, currently the 99th percentile size per UMA. Adding a single entry will collide with probability 23000 / 2^64, or about one in eight hundred trillion. Or, consider filling a cache with 23,000 entries. The probability of a single collision occuring is about: (-23000^2 / (2 * 2^64) ) -1.43E-11 -6.15E-12 1 - e ~= 1 - e ~= 1 - 10 which is about 0.0000000006%, or one in one hundred and seventy billion. A malicious attacker might also consider intentionally finding a collision between two URLs; perhaps one on a site they wish to attack, and a second on a site they control. Users who have visited their site would then be unable to load a particular resource from the target site for one load. To find this collision, our attacker would need to run, on average, 2^63 SHA1 operations. Running at 30GH/second, that's about ten years of CPU time. In the past twenty eight days of UMA, we've observed some collisions; about one in one hundred and eighty billion cache opens. But the UMA is hard to trust: if there's signal, it's very close to memory corruption rates. Because the key checking logic is now totally migrated back into the SimpleSynchronousEntry, we can return to reporting on key matching in the SyncOpenResult histograms. R=juliatuttle@chromium.org,asvitkine@chromium.org,rdsmith@chromium.org BUG=611732 ==========
On 2016/05/18 16:41:30, Randy Smith - Not in Fridays wrote: > Ok, at a detailed level I'm good with this CL, with the nits below. I'd like a > test that covers the "stream 0 read succeeds, stream 1 fails" case, even if it > isn't because of a hash collision; can you do that by creating an entry in which > the key and hash don't match? It looks like that isn't checked until > CheckHeaderAndKeyForOpen. > > However, I'm still chewing on the whole "hash collision can result in incorrect > headers being returned for a cache read" issue. I'm ok with that theoretically > with very low probability resulting in a request failure if the headers say to > revalidate, the source returns a 304, and we say "Whoops, we don't have it after > all". But I want to make sure that's our only failure mode, and I may pull in > some other eyes for that reason. Specific questions in that space: > > * Could you include a summary of your evaluation of the probabilities of > problems (both random and malicious) in the CL description? Done. > * Does failure in the above validate->read case doom the entry? Yes. That's done at the lowest level possible, the SimpleSynchronousEntry dooms itself immediately and the SimpleEntryImpl enters error mode (all operations thereafter fail). > * Would it make sense (not asking for it, just curious what you think) to > include a cross-hash (different function) of the key in the EOF records? Yes, but that is best done as a follow up CL. I think it would also be wise to use per-user salts on the hashes, if we want to mitigate the attack. > > We should probably talk about this verbally. > > https://codereview.chromium.org/1977863003/diff/80001/net/disk_cache/simple/s... > File net/disk_cache/simple/simple_synchronous_entry.cc (right): > > https://codereview.chromium.org/1977863003/diff/80001/net/disk_cache/simple/s... > net/disk_cache/simple/simple_synchronous_entry.cc:931: bool > SimpleSynchronousEntry::CheckHeaderAndKeyForOpen(int file_index) { > Given that this function is no longer being called during open, maybe a > different name? Maybe just deleting "For Open"? > > https://codereview.chromium.org/1977863003/diff/80001/net/disk_cache/simple/s... > net/disk_cache/simple/simple_synchronous_entry.cc:935: // mean an extra memory > to memory copy. > On 2016/05/18 15:46:00, gavinp wrote: > > On 2016/05/17 20:04:07, Randy Smith - Not in Fridays wrote: > > > Hmmm. The other issue I'd add to this comment (presuming you agree) is the > > > waste if kInitialHeaderRead actually reads into the entry; then it's not > just > > > the two separate reads, but the two reads doing the same thing. I presume > > that > > > won't happen often because you'll usually have the key_ and GetHeaderSize() > is > > > mostly reliable? > > > > Yeah, the key is almost always present. And in iteration, it's unlikely that > the > > entry will subsequently be read. More typically, it's doomed. > > > > Called it out. > > Ah, good point; I hadn't kept in mind no key -> iteration -> probably just > deleting based on time. You're welcome to go back to the old comment or not as > you choose. > > https://codereview.chromium.org/1977863003/diff/80001/net/disk_cache/simple/s... > net/disk_cache/simple/simple_synchronous_entry.cc:970: // The resize calls > invalidated |*header|, so it must be reset. > Why delete the comment? This was an expansion, so it may invalidate iterators. > > https://codereview.chromium.org/1977863003/diff/80001/net/disk_cache/simple/s... > net/disk_cache/simple/simple_synchronous_entry.cc:1013: > "simple-cache-dont-read-initial-header")) { > On 2016/05/17 20:04:07, Randy Smith - Not in Fridays wrote: > > On 2016/05/16 23:19:28, gavinp wrote: > > > Oh, yes, this must be removed for landing. I was using this for testing the > > > impact of this change using the disk cache perftool. > > > > Acknowledged. > > I'd feel better about it being gone in the PS I stamp? What's blocking its > removal? > > https://codereview.chromium.org/1977863003/diff/120001/net/disk_cache/simple/... > File net/disk_cache/simple/simple_entry_impl.cc (left): > > https://codereview.chromium.org/1977863003/diff/120001/net/disk_cache/simple/... > net/disk_cache/simple/simple_entry_impl.cc:1156: } > Why eliminate the DCHECK? > > https://codereview.chromium.org/1977863003/diff/120001/net/disk_cache/simple/... > File net/disk_cache/simple/simple_synchronous_entry.cc (right): > > https://codereview.chromium.org/1977863003/diff/120001/net/disk_cache/simple/... > net/disk_cache/simple/simple_synchronous_entry.cc:982: if (header_data.size() < > sizeof(*header) + header->key_length) { > Why do it by hand here and use GetHeaderSize() above? > > https://codereview.chromium.org/1977863003/diff/120001/net/disk_cache/simple/... > net/disk_cache/simple/simple_synchronous_entry.cc:982: if (header_data.size() < > sizeof(*header) + header->key_length) { > Suggestion: If I'm reading the code correctly, this if statement could be pulled > into the conditional of the above if statement (if the header_data wasn't too > small then, it won't be now either), which would allow you to return without > having to do the resize(). I think it would be a minor improvement in code > readability, even given the nested conditionals. But up to you.
https://codereview.chromium.org/1977863003/diff/80001/net/disk_cache/simple/s... File net/disk_cache/simple/simple_synchronous_entry.cc (right): https://codereview.chromium.org/1977863003/diff/80001/net/disk_cache/simple/s... net/disk_cache/simple/simple_synchronous_entry.cc:931: bool SimpleSynchronousEntry::CheckHeaderAndKeyForOpen(int file_index) { On 2016/05/18 16:41:30, Randy Smith - Not in Fridays wrote: > Given that this function is no longer being called during open, maybe a > different name? Maybe just deleting "For Open"? Well, it's still only for open... It's just that open completes later sometimes. But yes, it doesn't contribute meaning, so I'm removing it. https://codereview.chromium.org/1977863003/diff/80001/net/disk_cache/simple/s... net/disk_cache/simple/simple_synchronous_entry.cc:970: // The resize calls invalidated |*header|, so it must be reset. On 2016/05/18 16:41:30, Randy Smith - Not in Fridays wrote: > Why delete the comment? This was an expansion, so it may invalidate iterators. This was not an expansion, because of the resize a few lines above. It's gone now anyway, so moot. https://codereview.chromium.org/1977863003/diff/80001/net/disk_cache/simple/s... net/disk_cache/simple/simple_synchronous_entry.cc:1013: "simple-cache-dont-read-initial-header")) { On 2016/05/18 16:41:29, Randy Smith - Not in Fridays wrote: > On 2016/05/17 20:04:07, Randy Smith - Not in Fridays wrote: > > On 2016/05/16 23:19:28, gavinp wrote: > > > Oh, yes, this must be removed for landing. I was using this for testing the > > > impact of this change using the disk cache perftool. > > > > Acknowledged. > > I'd feel better about it being gone in the PS I stamp? What's blocking its > removal? Nothing, done. It was just convenient for me to use this CL to move it around platform to platform. But we're far enough along now that removing it is right. https://codereview.chromium.org/1977863003/diff/120001/net/disk_cache/simple/... File net/disk_cache/simple/simple_entry_impl.cc (left): https://codereview.chromium.org/1977863003/diff/120001/net/disk_cache/simple/... net/disk_cache/simple/simple_entry_impl.cc:1156: } On 2016/05/18 16:41:30, Randy Smith - Not in Fridays wrote: > Why eliminate the DCHECK? I put it back in, because an updated comment was documentary here. Updated the comment for not-wrongness. Most of the time, this will just be testing that std::string's copy constructor works, though; that's why I eliminated it. https://codereview.chromium.org/1977863003/diff/120001/net/disk_cache/simple/... File net/disk_cache/simple/simple_synchronous_entry.cc (right): https://codereview.chromium.org/1977863003/diff/120001/net/disk_cache/simple/... net/disk_cache/simple/simple_synchronous_entry.cc:982: if (header_data.size() < sizeof(*header) + header->key_length) { On 2016/05/18 16:41:30, Randy Smith - Not in Fridays wrote: > Suggestion: If I'm reading the code correctly, this if statement could be pulled > into the conditional of the above if statement (if the header_data wasn't too > small then, it won't be now either), which would allow you to return without > having to do the resize(). I think it would be a minor improvement in code > readability, even given the nested conditionals. But up to you. Good catch. Done.
Thanks! This looks good, but I want to just get an external consult to reduce the chances that there's some problematic header/data mismatch cases that you and I haven't caught. I'm going to chat with David at the end of the current meeting. Did you catch my request to explore doing a test for the "read stream 0 fail at stream 1 read" case that didn't involve hash collisions?
On 2016/05/18 19:42:15, Randy Smith - Not in Fridays wrote: > Thanks! This looks good, but I want to just get an external consult to reduce > the chances that there's some problematic header/data mismatch cases that you > and I haven't caught. I'm going to chat with David at the end of the current > meeting. > > Did you catch my request to explore doing a test for the "read stream 0 fail at > stream 1 read" case that didn't involve hash collisions? Whoops, I forgot. I'll add that tonight. I'm going to look at the cross-hash very closely as well.
On 2016/05/18 19:50:43, gavinp wrote: > On 2016/05/18 19:42:15, Randy Smith - Not in Fridays wrote: > > Thanks! This looks good, but I want to just get an external consult to reduce > > the chances that there's some problematic header/data mismatch cases that you > > and I haven't caught. I'm going to chat with David at the end of the current > > meeting. > > > > Did you catch my request to explore doing a test for the "read stream 0 fail > at > > stream 1 read" case that didn't involve hash collisions? > > Whoops, I forgot. I'll add that tonight. I'm going to look at the cross-hash > very closely as well. Ok, so I bent davidben@'s ear around this, and as I suspected, he was an excellent alternative perspective. My summary of his reaction, in order of his priority on them: * If we can rely on the cryptographic strength of the hash, we're fine--no worries. * If we're using SHA-1, could we consider switching to SHA-256? SHA-1 is fine for now, but is showing cracks, and now (before SimpleCache gets rolled out on all platforms) might be a really good time to switch. [I (Randy) would interpret this as "not needed for this CL, but would be nice to have a plan for in the immediate future".] * If we're *not* relying on the cryptographic strength of the hash and instead relying on the failure modes of collision not being too bad, he has some real concerns. Two possible problem with accessing a resource generating the wrong headers then failing on data read that he came up with were a) HSTS headers (he thought an attacker might be able to compromise HTTPS resources through this method), and b) the fetch API, where Javascript will get the headers before the data and could do anything it wanted with them. Those were just off the top of his head; he suspected there were more cases. [I think he was focussed on the case where the cache entry shows up as fresh and we pass the headers up to the consumer and only discover that they're the wrong headers if the data is then read, but he had some concern about how headers get combined in the 304 case you and I discussed as well.] I think that means this CL is fine, and I'll stamp it on that basis (LGTM) and let you do what you want with the S0 read succeeds->S1 read fails test, but I'd be curious about your thoughts about shifting to SHA-256.
On 2016/05/18 20:52:52, Randy Smith - Not in Fridays wrote: > On 2016/05/18 19:50:43, gavinp wrote: > > On 2016/05/18 19:42:15, Randy Smith - Not in Fridays wrote: > > > Thanks! This looks good, but I want to just get an external consult to > reduce > > > the chances that there's some problematic header/data mismatch cases that > you > > > and I haven't caught. I'm going to chat with David at the end of the > current > > > meeting. > > > > > > Did you catch my request to explore doing a test for the "read stream 0 fail > > at > > > stream 1 read" case that didn't involve hash collisions? > > > > Whoops, I forgot. I'll add that tonight. I'm going to look at the cross-hash > > very closely as well. > > Ok, so I bent davidben@'s ear around this, and as I suspected, he was an > excellent alternative perspective. My summary of his reaction, in order of his > priority on them: > * If we can rely on the cryptographic strength of the hash, we're fine--no > worries. > > * If we're using SHA-1, could we consider switching to SHA-256? SHA-1 is fine > for now, but is showing cracks, and now (before SimpleCache gets rolled out on > all platforms) might be a really good time to switch. [I (Randy) would > interpret this as "not needed for this CL, but would be nice to have a plan for > in the immediate future".] Hrm. I didn't realize we were using truncated hashes here. That's more of an issue then. If we are going to rely on cryptographic properties, the "there will never be collisions of a cryptographic hash ever" claim is only true if you don't truncate it. > * If we're *not* relying on the cryptographic strength of the hash and instead > relying on the failure modes of collision not being too bad, he has some real > concerns. Two possible problem with accessing a resource generating the wrong > headers then failing on data read that he came up with were a) HSTS headers (he > thought an attacker might be able to compromise HTTPS resources through this I forget exactly where we apply HSTS, so HSTS might not care. But it's an example of the sort of thing where integrity of headers is important too. > method), and b) the fetch API, where Javascript will get the headers before the > data and could do anything it wanted with them. Those were just off the top of > his head; he suspected there were more cases. [I think he was focussed on the > case where the cache entry shows up as fresh and we pass the headers up to the > consumer and only discover that they're the wrong headers if the data is then > read, but he had some concern about how headers get combined in the 304 case you > and I discussed as well.] > > I think that means this CL is fine, and I'll stamp it on that basis (LGTM) and > let you do what you want with the S0 read succeeds->S1 read fails test, but I'd > be curious about your thoughts about shifting to SHA-256.
On 2016/05/18 20:57:32, davidben (slow) wrote:
> On 2016/05/18 20:52:52, Randy Smith - Not in Fridays wrote:
> > On 2016/05/18 19:50:43, gavinp wrote:
> > > On 2016/05/18 19:42:15, Randy Smith - Not in Fridays wrote:
> > > > Thanks! This looks good, but I want to just get an external consult to
> > reduce
> > > > the chances that there's some problematic header/data mismatch cases
that
> > you
> > > > and I haven't caught. I'm going to chat with David at the end of the
> > current
> > > > meeting.
> > > >
> > > > Did you catch my request to explore doing a test for the "read stream 0
> fail
> > > at
> > > > stream 1 read" case that didn't involve hash collisions?
> > >
> > > Whoops, I forgot. I'll add that tonight. I'm going to look at the
cross-hash
> > > very closely as well.
> >
> > Ok, so I bent davidben@'s ear around this, and as I suspected, he was an
> > excellent alternative perspective. My summary of his reaction, in order of
> his
> > priority on them:
> > * If we can rely on the cryptographic strength of the hash, we're fine--no
> > worries.
> >
> > * If we're using SHA-1, could we consider switching to SHA-256? SHA-1 is
fine
> > for now, but is showing cracks, and now (before SimpleCache gets rolled out
on
> > all platforms) might be a really good time to switch. [I (Randy) would
> > interpret this as "not needed for this CL, but would be nice to have a plan
> for
> > in the immediate future".]
>
> Hrm. I didn't realize we were using truncated hashes here. That's more of an
> issue then. If we are going to rely on cryptographic properties, the "there
will
> never be collisions of a cryptographic hash ever" claim is only true if you
> don't truncate it.
>
> > * If we're *not* relying on the cryptographic strength of the hash and
instead
> > relying on the failure modes of collision not being too bad, he has some
real
> > concerns. Two possible problem with accessing a resource generating the
wrong
> > headers then failing on data read that he came up with were a) HSTS headers
> (he
> > thought an attacker might be able to compromise HTTPS resources through this
>
> I forget exactly where we apply HSTS, so HSTS might not care. But it's an
> example of the sort of thing where integrity of headers is important too.
>
> > method), and b) the fetch API, where Javascript will get the headers before
> the
> > data and could do anything it wanted with them. Those were just off the top
> of
> > his head; he suspected there were more cases. [I think he was focussed on
the
> > case where the cache entry shows up as fresh and we pass the headers up to
the
> > consumer and only discover that they're the wrong headers if the data is
then
> > read, but he had some concern about how headers get combined in the 304 case
> you
> > and I discussed as well.]
> >
> > I think that means this CL is fine, and I'll stamp it on that basis (LGTM)
and
> > let you do what you want with the S0 read succeeds->S1 read fails test, but
> I'd
> > be curious about your thoughts about shifting to SHA-256.
Arggh, so I jumped the gun. Gavin, please ignore my LGTM.
I'm not sure what the way forward here is. Gavin, can we include the whole
hash? That's probably a much bigger CL, though :-{.
On 2016/05/18 21:00:06, Randy Smith - Not in Fridays wrote:
> On 2016/05/18 20:57:32, davidben (slow) wrote:
> > On 2016/05/18 20:52:52, Randy Smith - Not in Fridays wrote:
> > > On 2016/05/18 19:50:43, gavinp wrote:
> > > > On 2016/05/18 19:42:15, Randy Smith - Not in Fridays wrote:
> > > > > Thanks! This looks good, but I want to just get an external consult
to
> > > reduce
> > > > > the chances that there's some problematic header/data mismatch cases
> that
> > > you
> > > > > and I haven't caught. I'm going to chat with David at the end of the
> > > current
> > > > > meeting.
> > > > >
> > > > > Did you catch my request to explore doing a test for the "read stream
0
> > fail
> > > > at
> > > > > stream 1 read" case that didn't involve hash collisions?
> > > >
> > > > Whoops, I forgot. I'll add that tonight. I'm going to look at the
> cross-hash
> > > > very closely as well.
> > >
> > > Ok, so I bent davidben@'s ear around this, and as I suspected, he was an
> > > excellent alternative perspective. My summary of his reaction, in order
of
> > his
> > > priority on them:
> > > * If we can rely on the cryptographic strength of the hash, we're fine--no
> > > worries.
> > >
> > > * If we're using SHA-1, could we consider switching to SHA-256? SHA-1 is
> fine
> > > for now, but is showing cracks, and now (before SimpleCache gets rolled
out
> on
> > > all platforms) might be a really good time to switch. [I (Randy) would
> > > interpret this as "not needed for this CL, but would be nice to have a
plan
> > for
> > > in the immediate future".]
> >
> > Hrm. I didn't realize we were using truncated hashes here. That's more of an
> > issue then. If we are going to rely on cryptographic properties, the "there
> will
> > never be collisions of a cryptographic hash ever" claim is only true if you
> > don't truncate it.
> >
> > > * If we're *not* relying on the cryptographic strength of the hash and
> instead
> > > relying on the failure modes of collision not being too bad, he has some
> real
> > > concerns. Two possible problem with accessing a resource generating the
> wrong
> > > headers then failing on data read that he came up with were a) HSTS
headers
> > (he
> > > thought an attacker might be able to compromise HTTPS resources through
this
> >
> > I forget exactly where we apply HSTS, so HSTS might not care. But it's an
> > example of the sort of thing where integrity of headers is important too.
> >
> > > method), and b) the fetch API, where Javascript will get the headers
before
> > the
> > > data and could do anything it wanted with them. Those were just off the
top
> > of
> > > his head; he suspected there were more cases. [I think he was focussed on
> the
> > > case where the cache entry shows up as fresh and we pass the headers up to
> the
> > > consumer and only discover that they're the wrong headers if the data is
> then
> > > read, but he had some concern about how headers get combined in the 304
case
> > you
> > > and I discussed as well.]
> > >
> > > I think that means this CL is fine, and I'll stamp it on that basis (LGTM)
> and
> > > let you do what you want with the S0 read succeeds->S1 read fails test,
but
> > I'd
> > > be curious about your thoughts about shifting to SHA-256.
>
> Arggh, so I jumped the gun. Gavin, please ignore my LGTM.
>
> I'm not sure what the way forward here is. Gavin, can we include the whole
> hash? That's probably a much bigger CL, though :-{.
I think another key question is whether or not it's ok to introduce a possible
security hole in an experiment, and if so, we could run this as an experiment as
is (well, ok, with finch control :-}) to validate the performance win, and then
update the format later, but before rolling it out more generally. But whether
it's ok to introduce a possible security hole in an experiment feels like a
judgement I can't make; possibly a quick ping to chrome-security?
On 2016/05/18 21:25:06, Randy Smith - Not in Fridays wrote:
> On 2016/05/18 21:00:06, Randy Smith - Not in Fridays wrote:
> > On 2016/05/18 20:57:32, davidben (slow) wrote:
> > > On 2016/05/18 20:52:52, Randy Smith - Not in Fridays wrote:
> > > > On 2016/05/18 19:50:43, gavinp wrote:
> > > > > On 2016/05/18 19:42:15, Randy Smith - Not in Fridays wrote:
> > > > > > Thanks! This looks good, but I want to just get an external consult
> to
> > > > reduce
> > > > > > the chances that there's some problematic header/data mismatch cases
> > that
> > > > you
> > > > > > and I haven't caught. I'm going to chat with David at the end of
the
> > > > current
> > > > > > meeting.
> > > > > >
> > > > > > Did you catch my request to explore doing a test for the "read
stream
> 0
> > > fail
> > > > > at
> > > > > > stream 1 read" case that didn't involve hash collisions?
> > > > >
> > > > > Whoops, I forgot. I'll add that tonight. I'm going to look at the
> > cross-hash
> > > > > very closely as well.
> > > >
> > > > Ok, so I bent davidben@'s ear around this, and as I suspected, he was an
> > > > excellent alternative perspective. My summary of his reaction, in order
> of
> > > his
> > > > priority on them:
> > > > * If we can rely on the cryptographic strength of the hash, we're
fine--no
> > > > worries.
> > > >
> > > > * If we're using SHA-1, could we consider switching to SHA-256? SHA-1
is
> > fine
> > > > for now, but is showing cracks, and now (before SimpleCache gets rolled
> out
> > on
> > > > all platforms) might be a really good time to switch. [I (Randy) would
> > > > interpret this as "not needed for this CL, but would be nice to have a
> plan
> > > for
> > > > in the immediate future".]
> > >
> > > Hrm. I didn't realize we were using truncated hashes here. That's more of
an
> > > issue then. If we are going to rely on cryptographic properties, the
"there
> > will
> > > never be collisions of a cryptographic hash ever" claim is only true if
you
> > > don't truncate it.
> > >
> > > > * If we're *not* relying on the cryptographic strength of the hash and
> > instead
> > > > relying on the failure modes of collision not being too bad, he has some
> > real
> > > > concerns. Two possible problem with accessing a resource generating the
> > wrong
> > > > headers then failing on data read that he came up with were a) HSTS
> headers
> > > (he
> > > > thought an attacker might be able to compromise HTTPS resources through
> this
> > >
> > > I forget exactly where we apply HSTS, so HSTS might not care. But it's an
> > > example of the sort of thing where integrity of headers is important too.
> > >
> > > > method), and b) the fetch API, where Javascript will get the headers
> before
> > > the
> > > > data and could do anything it wanted with them. Those were just off the
> top
> > > of
> > > > his head; he suspected there were more cases. [I think he was focussed
on
> > the
> > > > case where the cache entry shows up as fresh and we pass the headers up
to
> > the
> > > > consumer and only discover that they're the wrong headers if the data is
> > then
> > > > read, but he had some concern about how headers get combined in the 304
> case
> > > you
> > > > and I discussed as well.]
> > > >
> > > > I think that means this CL is fine, and I'll stamp it on that basis
(LGTM)
> > and
> > > > let you do what you want with the S0 read succeeds->S1 read fails test,
> but
> > > I'd
> > > > be curious about your thoughts about shifting to SHA-256.
> >
> > Arggh, so I jumped the gun. Gavin, please ignore my LGTM.
> >
> > I'm not sure what the way forward here is. Gavin, can we include the whole
> > hash? That's probably a much bigger CL, though :-{.
>
> I think another key question is whether or not it's ok to introduce a possible
> security hole in an experiment, and if so, we could run this as an experiment
as
> is (well, ok, with finch control :-}) to validate the performance win, and
then
> update the format later, but before rolling it out more generally. But
whether
> it's ok to introduce a possible security hole in an experiment feels like a
> judgement I can't make; possibly a quick ping to chrome-security?
Sorry, let me do a final message summing up my attitude about this CL, so I
impede you minimally with whatever you do next:
* I am good with it as written (with or without the test we discussed) from a
code point of view.
* I am concerned about the hash collision issue. I would be willing to consider
the CL good (i.e. you're welcome to use either of my stamps above) if you
changed the format so that we used all the bits of the hash.
* I suspect that's not going to allow it to get in to the current branch to
figure out if the performance advantages are good in the field. Given that, I'd
like you to get a review at the conceptual level from someone who's more
familiar with what security standard we want to hold to in chromium. If
(rsleevi || someone from chrome security) OKs the concept, I'm good. I could
imagine that they would require only running it in an experiment, changing
format, or be good with it as written; in all cases, I defer the security
evaluation to them.
Hopefully this will allow you to make forward progress without having to
synchronize with me, but I'll try to be available for synchronization anyway.
On 2016/05/18 20:57:32, davidben (slow) wrote: > On 2016/05/18 20:52:52, Randy Smith - Not in Fridays wrote: > > On 2016/05/18 19:50:43, gavinp wrote: > > > On 2016/05/18 19:42:15, Randy Smith - Not in Fridays wrote: > > > > Thanks! This looks good, but I want to just get an external consult to > > reduce > > > > the chances that there's some problematic header/data mismatch cases that > > you > > > > and I haven't caught. I'm going to chat with David at the end of the > > current > > > > meeting. > > > > > > > > Did you catch my request to explore doing a test for the "read stream 0 > fail > > > at > > > > stream 1 read" case that didn't involve hash collisions? > > > > > > Whoops, I forgot. I'll add that tonight. I'm going to look at the cross-hash > > > very closely as well. > > > > Ok, so I bent davidben@'s ear around this, and as I suspected, he was an > > excellent alternative perspective. My summary of his reaction, in order of > his > > priority on them: > > * If we can rely on the cryptographic strength of the hash, we're fine--no > > worries. > > > > * If we're using SHA-1, could we consider switching to SHA-256? SHA-1 is fine > > for now, but is showing cracks, and now (before SimpleCache gets rolled out on > > all platforms) might be a really good time to switch. [I (Randy) would > > interpret this as "not needed for this CL, but would be nice to have a plan > for > > in the immediate future".] > > Hrm. I didn't realize we were using truncated hashes here. That's more of an > issue then. If we are going to rely on cryptographic properties, the "there will > never be collisions of a cryptographic hash ever" claim is only true if you > don't truncate it. Truncation is explicitly endorsed by NIST, see NIST Special Publication 800-107, our truncation of SHA-1 should, by their rules, be expected to have second preimage resistance of 64 bits. But yes, if header integrity is important, we should validate the URL more solidly before serving them. > > * If we're *not* relying on the cryptographic strength of the hash and instead > > relying on the failure modes of collision not being too bad, he has some real > > concerns. Two possible problem with accessing a resource generating the wrong > > headers then failing on data read that he came up with were a) HSTS headers > (he > > thought an attacker might be able to compromise HTTPS resources through this > > I forget exactly where we apply HSTS, so HSTS might not care. But it's an > example of the sort of thing where integrity of headers is important too. > > > method), and b) the fetch API, where Javascript will get the headers before > the > > data and could do anything it wanted with them. Those were just off the top > of > > his head; he suspected there were more cases. [I think he was focussed on the > > case where the cache entry shows up as fresh and we pass the headers up to the > > consumer and only discover that they're the wrong headers if the data is then > > read, but he had some concern about how headers get combined in the 304 case > you > > and I discussed as well.] > > > > I think that means this CL is fine, and I'll stamp it on that basis (LGTM) and > > let you do what you want with the S0 read succeeds->S1 read fails test, but > I'd > > be curious about your thoughts about shifting to SHA-256.
Description was changed from
==========
SimpleCache: open with fewer seeks.
There's usually no reason to read the file header before reading the
stream 0 (headers) data when opening a simple cache entry. By not doing
so, seeks in the critical path of the loading headers are reduced, which
should speed up http caching performance.
To be careful, the headers are still checked when the first read of data
(if any) is performed. This delays detecting hash collisions in the
cache, because the header data is required to compare keys (instead of
just hashes).
A hash collision is still detected before the first read of the data
from the entry. But this still means that http requests can fail if
there's hash collisions. If there's an http request for a resource, and
the cache contains a collision, then the cache will give the wrong
headers to the HTTP Cache Transaction. If it then issues a conditional
request which 304s, the request will fail on the first read.
The hashes are 64 bit substrings of the SHA1 of the key.
To put the risk of collision in perspective, consider a cache with
23,000 entries, currently the 99th percentile size per UMA. Adding a
single entry will collide with probability 23000 / 2^64, or about
one in eight hundred trillion.
Or, consider filling a cache with 23,000 entries. The probability of a
single collision occuring is about:
(-23000^2 / (2 * 2^64) ) -1.43E-11 -6.15E-12
1 - e ~= 1 - e ~= 1 - 10
which is about 0.0000000006%, or one in one hundred and seventy billion.
A malicious attacker might also consider intentionally finding a
collision between two URLs; perhaps one on a site they wish to attack,
and a second on a site they control. Users who have visited their site
would then be unable to load a particular resource from the target site
for one load.
To find this collision, our attacker would need to run, on average, 2^63
SHA1 operations. Running at 30GH/second, that's about ten years of CPU
time.
In the past twenty eight days of UMA, we've observed some collisions;
about one in one hundred and eighty billion cache opens. But the UMA is
hard to trust: if there's signal, it's very close to memory corruption
rates.
Because the key checking logic is now totally migrated back into the
SimpleSynchronousEntry, we can return to reporting on key matching in
the SyncOpenResult histograms.
R=juliatuttle@chromium.org,asvitkine@chromium.org,rdsmith@chromium.org
BUG=611732
==========
to
==========
SimpleCache: open with fewer seeks.
There's usually no reason to read the file header before reading the
stream 0 (headers) data when opening a simple cache entry. By not doing
so, seeks in the critical path of the loading headers are reduced, which
should speed up http caching performance.
To be careful, the headers are still checked when the first read of data
(if any) is performed. This delays detecting hash collisions in the
cache, because the header data is required to compare keys (instead of
just hashes).
To compensate for the later key comparison, a SHA256 of the key is
stored as an optional prefix to the end of file record. This is used to
validate the key is correct before returning headers to the client.
Because the key checking logic is now totally migrated back into the
SimpleSynchronousEntry, we can return to reporting on key matching in
the SyncOpenResult histograms.
R=juliatuttle@chromium.org,asvitkine@chromium.org,rdsmith@chromium.org
BUG=611732
==========
avitkine: PTAL, new histogram. rdsmith: PTAL. This upload solves the hash collision --> bad HTTP headers making it out of the disk cache problem by a SHA256 of the key at the end of stream 0. When this is present, it allows the key to be strongly validated using end-of-file data only. When the key SHA256 is not present, there's a fallback to reading the headers and validating the key explicitly. I think there's a need for a few unit tests here; I'm working on those now. The two I think we need at least are: - Simple cache can open an entry that does not have the SHA256 record. - A mismatching key SHA256 prevents opening an entry. I'm writing those up now, I'll share them as soon as they are done.
lgtm
Ok. Thanks very much for doing this; this looks fairly solid to me. LGTM with the following notes: * Unless otherwise noted I would like to see the comment changes I call out below go in, but if for some reason you find yourself violently opposed to them and I'm not available to hash stuff out with, I don't have any urge to block landing on them. Do note that there are several commenting comments below that I'm being explicit about not feeling strongly about; those are completely up to your judgement. * I'm concerned about the issues I raise about recording UMA on incorrect file size and the total data size being negative in corner cases, but I am happy to trust your judgement as to the right remedial action there (if any) when you engage with the comments. In other words, at the end of the day, no conditions on the stamp :-}. Just please do engage with the comments below and use your best judgement. https://codereview.chromium.org/1977863003/diff/200001/net/disk_cache/simple/... File net/disk_cache/simple/simple_entry_format.h (right): https://codereview.chromium.org/1977863003/diff/200001/net/disk_cache/simple/... net/disk_cache/simple/simple_entry_format.h:30: // - a SimpleFileEOF record for stream 0. Update the comment by noting the possible suffix of the SHA-256 hash? https://codereview.chromium.org/1977863003/diff/200001/net/disk_cache/simple/... File net/disk_cache/simple/simple_synchronous_entry.cc (right): https://codereview.chromium.org/1977863003/diff/200001/net/disk_cache/simple/... net/disk_cache/simple/simple_synchronous_entry.cc:187: int64_t SimpleEntryStat::GetFileSize(size_t key_length, int file_index) const { Comment that this assumes the existence of the SHA256 and may be too large by that amount if it's not there? https://codereview.chromium.org/1977863003/diff/260001/net/disk_cache/simple/... File net/disk_cache/simple/simple_entry_format.h (right): https://codereview.chromium.org/1977863003/diff/260001/net/disk_cache/simple/... net/disk_cache/simple/simple_entry_format.h:50: FLAG_HAS_KEY_SHA256 = (2U << 1), Are you intentionally leaving a gap between the bits? If so, why? This also seems a confusing way to do it; is there a message you're trying to communicate by how you're setting the bit? (1U << 0 == 1, 2U << 1 == 4) https://codereview.chromium.org/1977863003/diff/260001/net/disk_cache/simple/... File net/disk_cache/simple/simple_synchronous_entry.cc (right): https://codereview.chromium.org/1977863003/diff/260001/net/disk_cache/simple/... net/disk_cache/simple/simple_synchronous_entry.cc:140: // Write SHA256 of the key immediately after stream 0 data. This comment seems like it's in the wrong place; this function doesn't know anything about the overall format of the file. https://codereview.chromium.org/1977863003/diff/260001/net/disk_cache/simple/... net/disk_cache/simple/simple_synchronous_entry.cc:1164: // EOF record for stream 0, which contains the size of stream 0. nit, very weak suggestions (especially because it's not really part of this CL): Rephrase to "Pretend the file has a null stream 0 to get the EOF record and find out how large stream 0 actually is". https://codereview.chromium.org/1977863003/diff/260001/net/disk_cache/simple/... net/disk_cache/simple/simple_synchronous_entry.cc:1169: total_data_size - sizeof(net::SHA256HashValue) - sizeof(SimpleFileEOF)); I'm reaching for a comment here to acknowledge the heisennature of the SHA256 value. Maybe "The final EOF record will be at the same place whether the hash is present or that space is stream data"? (Feel free not to use this, or anything else, if you can't find something you're happy enhances understanding--I"m just reaching for something to help the next person figure out exactly what's going on here more quickly than I did :-}.) https://codereview.chromium.org/1977863003/diff/260001/net/disk_cache/simple/... net/disk_cache/simple/simple_synchronous_entry.cc:1184: if (stream_1_size < 0) Hmmm. Do we record this in UMA somehow? This calculation is complex enough that I'd like some signal if we managed to get something wrong. https://codereview.chromium.org/1977863003/diff/280001/net/disk_cache/entry_u... File net/disk_cache/entry_unittest.cc (right): https://codereview.chromium.org/1977863003/diff/280001/net/disk_cache/entry_u... net/disk_cache/entry_unittest.cc:4185: EXPECT_EQ(net::OK, OpenEntry(key, &entry)); Confirm contents ok? (Probably need to write some :-}.) Preferably for both stream 0 & 1? I'd like to put in some protection against fencing errors. https://codereview.chromium.org/1977863003/diff/280001/net/disk_cache/simple/... File net/disk_cache/simple/simple_synchronous_entry.cc (right): https://codereview.chromium.org/1977863003/diff/280001/net/disk_cache/simple/... net/disk_cache/simple/simple_synchronous_entry.cc:197: int32_t total_data_size; (Suggestion for a followup CL; this problem predates this CL) I find the use of the "data size" concept in this function confusing; GetFileSizeFromDataSize() assumes that there's only a single stream in a file, and that's compensated for here by including in total_data_size the extra EOF record (and hash value, which is new) that's implied by the two stream file case. In my ideal world the variables and would be named in some way that makes that distinction clearer, possibly with a re-jigger of responsibility between functions. I'm not going to make specific suggestions since I don't think the fix belongs in this CL, but I wanted to point out another place where I did more tripping over figuring out what was going on than I'd ideally like to have in Chromium code. https://codereview.chromium.org/1977863003/diff/280001/net/disk_cache/simple/... net/disk_cache/simple/simple_synchronous_entry.cc:1169: total_data_size - sizeof(net::SHA256HashValue) - sizeof(SimpleFileEOF)); So I looked and didn't find any problems for this possibility, but might this value now be negative? I.e. if the hash isn't there, and the total data for streams 0 and 1 is, say, 0? I think everything works out ok if it is, but the thought scares me, so I wanted to run it by you in case you saw something I didn't. https://codereview.chromium.org/1977863003/diff/280001/net/disk_cache/simple/... File net/disk_cache/simple/simple_test_util.cc (right): https://codereview.chromium.org/1977863003/diff/280001/net/disk_cache/simple/... net/disk_cache/simple/simple_test_util.cc:54: file_length - sizeof(eof_record) - sizeof(net::SHA256HashValue), Comment calling out that record is being moved back by sizeof(net::SHA256HashValue)? It find it easy to miss things like that when reading a wall of code.
On 2016/05/19 19:38:50, Randy Smith - Not in Fridays wrote: > Ok. Thanks very much for doing this; this looks fairly solid to me. LGTM with > the following notes: > * Unless otherwise noted I would like to see the comment changes I call out > below go in, but if for some reason you find yourself violently opposed to them > and I'm not available to hash stuff out with, I don't have any urge to block > landing on them. > > Do note that there are several commenting comments below that I'm being explicit > about not feeling strongly about; those are completely up to your judgement. > > * I'm concerned about the issues I raise about recording UMA on incorrect file > size and the total data size being negative in corner cases, but I am happy to > trust your judgement as to the right remedial action there (if any) when you > engage with the comments. > > In other words, at the end of the day, no conditions on the stamp :-}. Just > please do engage with the comments below and use your best judgement. > > https://codereview.chromium.org/1977863003/diff/200001/net/disk_cache/simple/... > File net/disk_cache/simple/simple_entry_format.h (right): > > https://codereview.chromium.org/1977863003/diff/200001/net/disk_cache/simple/... > net/disk_cache/simple/simple_entry_format.h:30: // - a SimpleFileEOF record > for stream 0. > Update the comment by noting the possible suffix of the SHA-256 hash? > > https://codereview.chromium.org/1977863003/diff/200001/net/disk_cache/simple/... > File net/disk_cache/simple/simple_synchronous_entry.cc (right): > > https://codereview.chromium.org/1977863003/diff/200001/net/disk_cache/simple/... > net/disk_cache/simple/simple_synchronous_entry.cc:187: int64_t > SimpleEntryStat::GetFileSize(size_t key_length, int file_index) const { > Comment that this assumes the existence of the SHA256 and may be too large by > that amount if it's not there? > > https://codereview.chromium.org/1977863003/diff/260001/net/disk_cache/simple/... > File net/disk_cache/simple/simple_entry_format.h (right): > > https://codereview.chromium.org/1977863003/diff/260001/net/disk_cache/simple/... > net/disk_cache/simple/simple_entry_format.h:50: FLAG_HAS_KEY_SHA256 = (2U << 1), > Are you intentionally leaving a gap between the bits? If so, why? > > This also seems a confusing way to do it; is there a message you're trying to > communicate by how you're setting the bit? > > (1U << 0 == 1, 2U << 1 == 4) > > https://codereview.chromium.org/1977863003/diff/260001/net/disk_cache/simple/... > File net/disk_cache/simple/simple_synchronous_entry.cc (right): > > https://codereview.chromium.org/1977863003/diff/260001/net/disk_cache/simple/... > net/disk_cache/simple/simple_synchronous_entry.cc:140: // Write SHA256 of the > key immediately after stream 0 data. > This comment seems like it's in the wrong place; this function doesn't know > anything about the overall format of the file. > > https://codereview.chromium.org/1977863003/diff/260001/net/disk_cache/simple/... > net/disk_cache/simple/simple_synchronous_entry.cc:1164: // EOF record for stream > 0, which contains the size of stream 0. > nit, very weak suggestions (especially because it's not really part of this CL): > Rephrase to "Pretend the file has a null stream 0 to get the EOF record and find > out how large stream 0 actually is". > > https://codereview.chromium.org/1977863003/diff/260001/net/disk_cache/simple/... > net/disk_cache/simple/simple_synchronous_entry.cc:1169: total_data_size - > sizeof(net::SHA256HashValue) - sizeof(SimpleFileEOF)); > I'm reaching for a comment here to acknowledge the heisennature of the SHA256 > value. Maybe "The final EOF record will be at the same place whether the hash > is present or that space is stream data"? (Feel free not to use this, or > anything else, if you can't find something you're happy enhances > understanding--I"m just reaching for something to help the next person figure > out exactly what's going on here more quickly than I did :-}.) > > https://codereview.chromium.org/1977863003/diff/260001/net/disk_cache/simple/... > net/disk_cache/simple/simple_synchronous_entry.cc:1184: if (stream_1_size < 0) > Hmmm. Do we record this in UMA somehow? This calculation is complex enough > that I'd like some signal if we managed to get something wrong. > > https://codereview.chromium.org/1977863003/diff/280001/net/disk_cache/entry_u... > File net/disk_cache/entry_unittest.cc (right): > > https://codereview.chromium.org/1977863003/diff/280001/net/disk_cache/entry_u... > net/disk_cache/entry_unittest.cc:4185: EXPECT_EQ(net::OK, OpenEntry(key, > &entry)); > Confirm contents ok? (Probably need to write some :-}.) Preferably for both > stream 0 & 1? I'd like to put in some protection against fencing errors. > > https://codereview.chromium.org/1977863003/diff/280001/net/disk_cache/simple/... > File net/disk_cache/simple/simple_synchronous_entry.cc (right): > > https://codereview.chromium.org/1977863003/diff/280001/net/disk_cache/simple/... > net/disk_cache/simple/simple_synchronous_entry.cc:197: int32_t total_data_size; > (Suggestion for a followup CL; this problem predates this CL) > > I find the use of the "data size" concept in this function confusing; > GetFileSizeFromDataSize() assumes that there's only a single stream in a file, > and that's compensated for here by including in total_data_size the extra EOF > record (and hash value, which is new) that's implied by the two stream file > case. In my ideal world the variables and would be named in some way that makes > that distinction clearer, possibly with a re-jigger of responsibility between > functions. I'm not going to make specific suggestions since I don't think the > fix belongs in this CL, but I wanted to point out another place where I did more > tripping over figuring out what was going on than I'd ideally like to have in > Chromium code. > > https://codereview.chromium.org/1977863003/diff/280001/net/disk_cache/simple/... > net/disk_cache/simple/simple_synchronous_entry.cc:1169: total_data_size - > sizeof(net::SHA256HashValue) - sizeof(SimpleFileEOF)); > So I looked and didn't find any problems for this possibility, but might this > value now be negative? I.e. if the hash isn't there, and the total data for > streams 0 and 1 is, say, 0? I think everything works out ok if it is, but the > thought scares me, so I wanted to run it by you in case you saw something I > didn't. > > https://codereview.chromium.org/1977863003/diff/280001/net/disk_cache/simple/... > File net/disk_cache/simple/simple_test_util.cc (right): > > https://codereview.chromium.org/1977863003/diff/280001/net/disk_cache/simple/... > net/disk_cache/simple/simple_test_util.cc:54: file_length - sizeof(eof_record) - > sizeof(net::SHA256HashValue), > Comment calling out that record is being moved back by > sizeof(net::SHA256HashValue)? It find it easy to miss things like that when > reading a wall of code. Sorry, one other high level comment suggestion: I think it would be good to say somewhere (simple_entry_format.h?) that we always validate the File header, including doing a key comparison if we have a key, before returning stream 1 & 2 data, and that stream 0 data will be validated against the SHA256 hash if we have it, and the file header + key compare if possible if we don't have the hash. It's the kind of framing comment that can make reading this kind of code much easier.
I've addressed most of your comments. I think you're right to point out the need for a follow-up CL to make sure we watch all exit paths from a synchronous entry open, it's gnarly that there's failure modes that are not tracked. https://codereview.chromium.org/1977863003/diff/200001/net/disk_cache/simple/... File net/disk_cache/simple/simple_entry_format.h (right): https://codereview.chromium.org/1977863003/diff/200001/net/disk_cache/simple/... net/disk_cache/simple/simple_entry_format.h:30: // - a SimpleFileEOF record for stream 0. On 2016/05/19 19:38:49, Randy Smith - Not in Fridays wrote: > Update the comment by noting the possible suffix of the SHA-256 hash? Done. https://codereview.chromium.org/1977863003/diff/200001/net/disk_cache/simple/... File net/disk_cache/simple/simple_synchronous_entry.cc (right): https://codereview.chromium.org/1977863003/diff/200001/net/disk_cache/simple/... net/disk_cache/simple/simple_synchronous_entry.cc:187: int64_t SimpleEntryStat::GetFileSize(size_t key_length, int file_index) const { On 2016/05/19 19:38:49, Randy Smith - Not in Fridays wrote: > Comment that this assumes the existence of the SHA256 and may be too large by > that amount if it's not there? I added a longer comment above all of these explaining why it's presumed to be present. https://codereview.chromium.org/1977863003/diff/260001/net/disk_cache/simple/... File net/disk_cache/simple/simple_entry_format.h (right): https://codereview.chromium.org/1977863003/diff/260001/net/disk_cache/simple/... net/disk_cache/simple/simple_entry_format.h:50: FLAG_HAS_KEY_SHA256 = (2U << 1), On 2016/05/19 19:38:49, Randy Smith - Not in Fridays wrote: > Are you intentionally leaving a gap between the bits? If so, why? > > This also seems a confusing way to do it; is there a message you're trying to > communicate by how you're setting the bit? > > (1U << 0 == 1, 2U << 1 == 4) Nope, that was a mistake. Thanks. https://codereview.chromium.org/1977863003/diff/260001/net/disk_cache/simple/... File net/disk_cache/simple/simple_synchronous_entry.cc (right): https://codereview.chromium.org/1977863003/diff/260001/net/disk_cache/simple/... net/disk_cache/simple/simple_synchronous_entry.cc:140: // Write SHA256 of the key immediately after stream 0 data. On 2016/05/19 19:38:49, Randy Smith - Not in Fridays wrote: > This comment seems like it's in the wrong place; this function doesn't know > anything about the overall format of the file. Yeah, it got cut and paste here. Whoops. https://codereview.chromium.org/1977863003/diff/260001/net/disk_cache/simple/... net/disk_cache/simple/simple_synchronous_entry.cc:1164: // EOF record for stream 0, which contains the size of stream 0. On 2016/05/19 19:38:49, Randy Smith - Not in Fridays wrote: > nit, very weak suggestions (especially because it's not really part of this CL): > Rephrase to "Pretend the file has a null stream 0 to get the EOF record and find > out how large stream 0 actually is". Done. https://codereview.chromium.org/1977863003/diff/260001/net/disk_cache/simple/... net/disk_cache/simple/simple_synchronous_entry.cc:1169: total_data_size - sizeof(net::SHA256HashValue) - sizeof(SimpleFileEOF)); On 2016/05/19 19:38:49, Randy Smith - Not in Fridays wrote: > I'm reaching for a comment here to acknowledge the heisennature of the SHA256 > value. Maybe "The final EOF record will be at the same place whether the hash > is present or that space is stream data"? (Feel free not to use this, or > anything else, if you can't find something you're happy enhances > understanding--I"m just reaching for something to help the next person figure > out exactly what's going on here more quickly than I did :-}.) I put this in the comment edit above, describing the assumptions made to let the read move forward. https://codereview.chromium.org/1977863003/diff/260001/net/disk_cache/simple/... net/disk_cache/simple/simple_synchronous_entry.cc:1184: if (stream_1_size < 0) On 2016/05/19 19:38:49, Randy Smith - Not in Fridays wrote: > Hmmm. Do we record this in UMA somehow? This calculation is complex enough > that I'd like some signal if we managed to get something wrong. No, it's not. And it hasn't ever been. I'll write a quick followup CL to add this, I agree it's important to track. https://codereview.chromium.org/1977863003/diff/280001/net/disk_cache/entry_u... File net/disk_cache/entry_unittest.cc (right): https://codereview.chromium.org/1977863003/diff/280001/net/disk_cache/entry_u... net/disk_cache/entry_unittest.cc:4185: EXPECT_EQ(net::OK, OpenEntry(key, &entry)); On 2016/05/19 19:38:50, Randy Smith - Not in Fridays wrote: > Confirm contents ok? (Probably need to write some :-}.) Preferably for both > stream 0 & 1? I'd like to put in some protection against fencing errors. Done. https://codereview.chromium.org/1977863003/diff/280001/net/disk_cache/simple/... File net/disk_cache/simple/simple_synchronous_entry.cc (right): https://codereview.chromium.org/1977863003/diff/280001/net/disk_cache/simple/... net/disk_cache/simple/simple_synchronous_entry.cc:197: int32_t total_data_size; On 2016/05/19 19:38:50, Randy Smith - Not in Fridays wrote: > (Suggestion for a followup CL; this problem predates this CL) > > I find the use of the "data size" concept in this function confusing; > GetFileSizeFromDataSize() assumes that there's only a single stream in a file, > and that's compensated for here by including in total_data_size the extra EOF > record (and hash value, which is new) that's implied by the two stream file > case. In my ideal world the variables and would be named in some way that makes > that distinction clearer, possibly with a re-jigger of responsibility between > functions. I'm not going to make specific suggestions since I don't think the > fix belongs in this CL, but I wanted to point out another place where I did more > tripping over figuring out what was going on than I'd ideally like to have in > Chromium code. I agree. this has been a problem for a long time. https://codereview.chromium.org/1977863003/diff/280001/net/disk_cache/simple/... net/disk_cache/simple/simple_synchronous_entry.cc:1169: total_data_size - sizeof(net::SHA256HashValue) - sizeof(SimpleFileEOF)); On 2016/05/19 19:38:50, Randy Smith - Not in Fridays wrote: > So I looked and didn't find any problems for this possibility, but might this > value now be negative? I.e. if the hash isn't there, and the total data for > streams 0 and 1 is, say, 0? I think everything works out ok if it is, but the > thought scares me, so I wanted to run it by you in case you saw something I > didn't. It is negative occasionally, I believe -7 at most (least?). It's clean, some unit tests exercise this path. https://codereview.chromium.org/1977863003/diff/280001/net/disk_cache/simple/... File net/disk_cache/simple/simple_test_util.cc (right): https://codereview.chromium.org/1977863003/diff/280001/net/disk_cache/simple/... net/disk_cache/simple/simple_test_util.cc:54: file_length - sizeof(eof_record) - sizeof(net::SHA256HashValue), On 2016/05/19 19:38:50, Randy Smith - Not in Fridays wrote: > Comment calling out that record is being moved back by > sizeof(net::SHA256HashValue)? It find it easy to miss things like that when > reading a wall of code. Done.
The CQ bit was checked by gavinp@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from asvitkine@chromium.org, rdsmith@chromium.org Link to the patchset: https://codereview.chromium.org/1977863003/#ps320001 (title: "speeeling")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1977863003/320001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1977863003/320001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_asan_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by gavinp@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from asvitkine@chromium.org, rdsmith@chromium.org Link to the patchset: https://codereview.chromium.org/1977863003/#ps340001 (title: "fix leak in unittest")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1977863003/340001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1977863003/340001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by gavinp@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from asvitkine@chromium.org, rdsmith@chromium.org Link to the patchset: https://codereview.chromium.org/1977863003/#ps380001 (title: "actually fix test")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1977863003/380001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1977863003/380001
Message was sent while issue was closed.
Description was changed from ========== SimpleCache: open with fewer seeks. There's usually no reason to read the file header before reading the stream 0 (headers) data when opening a simple cache entry. By not doing so, seeks in the critical path of the loading headers are reduced, which should speed up http caching performance. To be careful, the headers are still checked when the first read of data (if any) is performed. This delays detecting hash collisions in the cache, because the header data is required to compare keys (instead of just hashes). To compensate for the later key comparison, a SHA256 of the key is stored as an optional prefix to the end of file record. This is used to validate the key is correct before returning headers to the client. Because the key checking logic is now totally migrated back into the SimpleSynchronousEntry, we can return to reporting on key matching in the SyncOpenResult histograms. R=juliatuttle@chromium.org,asvitkine@chromium.org,rdsmith@chromium.org BUG=611732 ========== to ========== SimpleCache: open with fewer seeks. There's usually no reason to read the file header before reading the stream 0 (headers) data when opening a simple cache entry. By not doing so, seeks in the critical path of the loading headers are reduced, which should speed up http caching performance. To be careful, the headers are still checked when the first read of data (if any) is performed. This delays detecting hash collisions in the cache, because the header data is required to compare keys (instead of just hashes). To compensate for the later key comparison, a SHA256 of the key is stored as an optional prefix to the end of file record. This is used to validate the key is correct before returning headers to the client. Because the key checking logic is now totally migrated back into the SimpleSynchronousEntry, we can return to reporting on key matching in the SyncOpenResult histograms. R=juliatuttle@chromium.org,asvitkine@chromium.org,rdsmith@chromium.org BUG=611732 ==========
Message was sent while issue was closed.
Committed patchset #20 (id:380001)
Message was sent while issue was closed.
Description was changed from ========== SimpleCache: open with fewer seeks. There's usually no reason to read the file header before reading the stream 0 (headers) data when opening a simple cache entry. By not doing so, seeks in the critical path of the loading headers are reduced, which should speed up http caching performance. To be careful, the headers are still checked when the first read of data (if any) is performed. This delays detecting hash collisions in the cache, because the header data is required to compare keys (instead of just hashes). To compensate for the later key comparison, a SHA256 of the key is stored as an optional prefix to the end of file record. This is used to validate the key is correct before returning headers to the client. Because the key checking logic is now totally migrated back into the SimpleSynchronousEntry, we can return to reporting on key matching in the SyncOpenResult histograms. R=juliatuttle@chromium.org,asvitkine@chromium.org,rdsmith@chromium.org BUG=611732 ========== to ========== SimpleCache: open with fewer seeks. There's usually no reason to read the file header before reading the stream 0 (headers) data when opening a simple cache entry. By not doing so, seeks in the critical path of the loading headers are reduced, which should speed up http caching performance. To be careful, the headers are still checked when the first read of data (if any) is performed. This delays detecting hash collisions in the cache, because the header data is required to compare keys (instead of just hashes). To compensate for the later key comparison, a SHA256 of the key is stored as an optional prefix to the end of file record. This is used to validate the key is correct before returning headers to the client. Because the key checking logic is now totally migrated back into the SimpleSynchronousEntry, we can return to reporting on key matching in the SyncOpenResult histograms. R=juliatuttle@chromium.org,asvitkine@chromium.org,rdsmith@chromium.org BUG=611732 Committed: https://crrev.com/9f15f2258d7cae8cd12cb4ef7143ad648a958333 Cr-Commit-Position: refs/heads/master@{#395083} ==========
Message was sent while issue was closed.
Patchset 20 (id:??) landed as https://crrev.com/9f15f2258d7cae8cd12cb4ef7143ad648a958333 Cr-Commit-Position: refs/heads/master@{#395083} |
