Based on bug 813935 (remove offset from diskCache): for DiskCacheOutputStream, only use the buffer for the first 16K bytes (which can still fit in the cacheblocks storage), and after that write directly to the cache file. This reduces the risk of crashing on the buffer (as the code is much simpler), and increases performance as it reduces a buffer copy for large files. Note, writes have sizes up to 32K, in which case writing through a buffer of 16K is particular painfull. Also, this ensures that every write (after the first 16K) is directly written to the cache file (besides OS buffering for it) and is admin'd as such in the cache entry.

See https://tbpl.mozilla.org/?tree=Try&rev=0f9e43ff5fc3 for tryrun details.

Tryrun fully green: https://tbpl.mozilla.org/?tree=Try&rev=5cb5a363984c Except for two intermittent issues, a phonecall issues, and two libnss3.so crashes: libnss3.so!nssCertificate_Destroy [certificate.c : 102 + 0x0] and libnss3.so!CERT_DestroyCertificate [stanpcertdb.c : 797 + 0x3]

Alfred/Michal - how risky do you think this would be to uplift to FF19, if it proves to help with bug 572011?

Any change to the cache code is risky, as it is to very core of Gecko. But this patch survived all testruns. So the next step would be to apply it to nightly/aurora to give it more burn time.

Patch needs rebasing due to bug 808997.

Comment on attachment 693373 [details] [diff] [review] V4: Remove a stray NS_ASSERTION The patch looks good in general. Though, one thing I don't like about this patch is that it changes the logic of selection where to store the data when updating the entry. When the entry is stored in separate file and the new content would fit into block file, then with the current code we would try to save it there. With this patch the data stays in the separate file. > + // We have more data than the current buffer size? > + if ((mStreamEnd + count >= mBufSize) && (mBufSize < kMaxBufferSize)) { Why not just '>' ? ^^^^ Also, please keep the length of new and changed lines at 80 chars max.

try: -b do -p all -u all -t none https://tbpl.mozilla.org/?tree=Try&rev=e75498192d0b All green (except for the not cache related known intermittent and automation errors)

Comment on attachment 693373 [details] [diff] [review] V4: Remove a stray NS_ASSERTION (In reply to Michal Novotny (:michal) from comment #10) > Though, one thing I don't like about this patch is that it changes the logic > of selection where to store the data when updating the entry. When the entry > is stored in separate file and the new content would fit into block file, > then with the current code we would try to save it there. With this patch the > data stays in the separate file. What about fixing this in nsDiskCacheStreamIO::SeekAndTruncate(). In case offset is 0 (which is most of the time) and we have already a separate file, we could delete the file and clear data location instead of truncating the file.

Good idea, I have submitted this to teh tryserver to test it out.

I think we may need to uplift to Aurora in order to verify this has had an impact on the overall crash volume in bug 572011.

I agree. If so, then also bug 815639 should be also uplifted

Whoops, this latest change was landed in FF20 (which is now on Aurora). Sadly, bug 572011 still appears high in volume in Aurora.

Attached to bug 527011 is a patch which should fix that crash.