828281 - UpdateRecord in CloseOutputStream can be removed

Status: RESOLVED FIXED

(Core :: Networking: Cache, defect)

Description

[Alfred Kayser]

This is the line in nsDiskCacheStream.cpp
514     // XXX do we need this here?  WriteDataCacheBlocks() calls UpdateRecord()

As the buffering logic has been simplified, the flush either writes data to the cacheblocks, which also performs the UpdateRecord, or the file which then calls UpdateFileSize, which calls UpdateRecord.

UpdateRecord is not a light function, and calls InvalidateCache(), etc...

Comment 1

[Alfred Kayser]

Attachment: Patch to only call UpdateRecord when needed

UpdateRecord is not needed when calling WriteDataCacheBlocks, but it is needed when there is no data (mStreamEnd==0), so only call UpdateRecord in that scenario.
Furthermore structure the code to follow the logic:
1. if writing to file, close file and return else the stream is small enough to fit in blocks storage.
3. delete any old storage (file or blocks)
4.

Comment 2

[Alfred Kayser]

UpdateRecord is not needed when calling WriteDataCacheBlocks, but it is needed when there is no data (mStreamEnd==0), so only call UpdateRecord in that scenario.
Furthermore structure the code to follow the logic:
1. if writing to file, close file and return 
else the stream is small enough to fit in blocks storage.:
2. delete any old storage (file or blocks)
3. if no data, just update record and return
4. write to blocks
5. if failed, flush to file.

Overall effect is that UpdateRecord is not called twice when writing to blocks (when that fails, flushing to file). 
UpdateRecord triggers InvalidateCache, which performs a file write (of 1 byte) and setting a timer, so it is somewhat costly to call twice.

tryserver is running:
https://tbpl.mozilla.org/?tree=Try&rev=842d8a7d1901

Comment 3

[Mozilla RelEng Bot]

Try run for 842d8a7d1901 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=842d8a7d1901
Results (out of 229 total builds):
    exception: 5
    success: 199
    warnings: 24
    failure: 1
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-842d8a7d1901

Comment 4

[Michal Novotny [:michal]]

Comment on attachment 700247 [details] [diff] [review]
Patch to only call UpdateRecord when needed

Looks good, r+ with following nits fixed:


> +        nsresult rv = cacheMap->DeleteStorage(record, nsDiskCache::kData);

There are 3 declaration of rv. Do it only once at the beginning of the method.


> -        NS_WARNING("cacheMap->UpdateRecord() failed.");
> -        return rv;   // XXX doom cache entry

You've moved the code without the comment. Either keep the comment, or check what exactly happens in case of failure and whether we should or shouldn't doom the entry.

Comment 5

[Alfred Kayser]

Attachment: V2: addressed review comments

https://tbpl.mozilla.org/?tree=Try&rev=f0ebade21120

Comment 6

[Alfred Kayser]

https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=4fe38a0ee8c5

Comment 7

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/4fe38a0ee8c5