Closed Bug 814010 Opened 7 years ago Closed 7 years ago

diskcache: skip buffer when writing to file (preventing crashes on the buffer)

Categories

(Core :: Networking: Cache, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla20

People

(Reporter: alfredkayser, Assigned: alfredkayser)

References

Details

(Keywords: perf)

Attachments

(1 file, 5 obsolete files)

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.
Assignee: nobody → alfredkayser
Status: NEW → ASSIGNED
Attachment #684032 - Attachment is patch: true
Blocks: 572011
Depends on: 813935
Attached patch Tryrun version 2 (obsolete) — Splinter Review
See https://tbpl.mozilla.org/?tree=Try&rev=0f9e43ff5fc3 for tryrun details.
Attachment #684032 - Attachment is obsolete: true
Attachment #684602 - Attachment is obsolete: true
Blocks: 405407
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]
Attachment #685097 - Flags: review?(joshmoz)
Blocks: 804614
Blocks: 810781
Blocks: 812853
Attachment #685097 - Flags: review?(joshmoz) → review?(michal.novotny)
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.
Depends on: 808997
Patch needs rebasing due to bug 808997.
Depends on: 725993
Attached patch Rebased after bug 725993 (obsolete) — Splinter Review
Attachment #685097 - Attachment is obsolete: true
Attachment #685097 - Flags: review?(michal.novotny)
Attachment #692726 - Flags: review?(michal.novotny)
Attachment #692726 - Attachment is patch: true
Attached patch V4: Remove a stray NS_ASSERTION (obsolete) — Splinter Review
Attachment #692726 - Attachment is obsolete: true
Attachment #692726 - Flags: review?(michal.novotny)
Attachment #693373 - Flags: review?(michal.novotny)
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.
Attachment #693373 - Flags: review?(michal.novotny)
Good idea, I have submitted this to teh tryserver to test it out.
Attachment #693373 - Attachment is obsolete: true
Attachment #694704 - Flags: review?(michal.novotny)
Blocks: 824244
Keywords: perf
Attachment #694704 - Flags: review?(michal.novotny) → review+
https://hg.mozilla.org/mozilla-central/rev/c4abfca219e5
Target Milestone: --- → mozilla20
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.
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.