It does not appear that we use content-length to produce setfilesize/fallocate calls prior to writing out to the cache files. Hopefully this is an easy change.

Should be a one or two liner, could have a decent perf payoff, esp for startup.

Michal, can you write up this patch?

Comment on attachment 477117 [details] [diff] [review] fix >+ >+ PRInt64 dataSize = mBinding->mCacheEntry->PredictedDataSize(); >+ if (dataSize != -1) >+ mozilla::fallocate(mFD, dataSize); > } I think preallocation be capped at 1-4mb. We basically want to make sure that data within webpages such as html,images,css,js,etc are laid out continuously. It matters less if a long youtube video is laid out continuously. We should also truncate files before closing them to make sure they aren't taking up more space than the data within them.

(In reply to comment #4) > > I think preallocation be capped at 1-4mb. We basically want to make sure that > data within webpages such as html,images,css,js,etc are laid out continuously. > It matters less if a long youtube video is laid out continuously. For what its worth, entries greater than 5MB are not allowed in the cache. So we would never end up preallocating more than that.

However, we might still impose the cap, in case this setting changes in the future...

I've chosen 1MB. Feel free to suggest different limit.

Comment on attachment 477326 [details] [diff] [review] patch v2 >+ >+ PRInt64 dataSize = mBinding->mCacheEntry->PredictedDataSize(); >+ if ((dataSize != -1) && (dataSize <= kPreallocateLimit)) >+ mozilla::fallocate(mFD, dataSize); I meant mozilla::fallocate(mFD, PR_MIN(dataSize,kPreallocateLimit));

Taras, Why not just use fallocate() for the full length, always? I don't see why we want a size limit here. Fallocate does not guarantee contiguous space--it merely guarantees that disk space is available, and allows the OS to optimize on that info. So it's not like we're going to fail if there isn't a contiguous set of blocks on disk. The only downside to this approach is that for large files, on older filesystems (ex: ext3), fallocate is not much faster than writing zero's to the entire file. See http://tinyurl.com/24o7csh He sees times of almost 100 seconds to fallocate a 1 GB file on ext3. I assume FAT32 may be similar. Assuming that reduces linearly to smaller files, we're talking about 100 ms for a 5 MB file. That might be enough time to stall the network connection if TCP socket buffers fill up while we wait for the cache entry being writable (assuming there's a blocking dependency there, which I should know, but don't). Not sure what the right tradeoff is here (how common are ext3/FAT32 these days?). If we decide to fallocate a smaller size than the current max (5 MB), should we keep track of how many bytes have been written, and fallocate another block again when we exceed the initial size? Slight tangent: do we want to open a bug for lazy creation of our CACHE_00x files? Right now they all get created at once (in OpenBlockFiles), the first time any cache entry is requested (which is usually pretty early during startup), and at 20 MB each for three files, that could block cache access for 1200 ms (or longer once we have more files for larger block sizes). OTOH, this only happens when you launch for the very first time, or after a crash (or cache version update, like FF4). On ext4 and other new filesystems, fallocate is essentially free, so this is all moot. I'm not sure how SetEndOfFile (the win32 equivalent) does on NTFS.

(In reply to comment #10) > Taras, > > Why not just use fallocate() for the full length, always? I don't see why we > want a size limit here. Fallocate does not guarantee contiguous space--it > merely guarantees that disk space is available, and allows the OS to optimize > on that info. So it's not like we're going to fail if there isn't a > contiguous set of blocks on disk. I am worried about allocating 5mb for an entry that might fail to download due to possibility of wasting disk space. I think it's safe to assume that most objects used with a webpage are < 1mb. > > The only downside to this approach is that for large files, on older > filesystems (ex: ext3), fallocate is not much faster than writing zero's to the > entire file. See > > http://tinyurl.com/24o7csh > > He sees times of almost 100 seconds to fallocate a 1 GB file on ext3. I assume > FAT32 may be similar. Assuming that reduces linearly to smaller files, we're > talking about 100 ms for a 5 MB file. That might be enough time to stall the > network connection if TCP socket buffers fill up while we wait for the cache > entry being writable (assuming there's a blocking dependency there, which I > should know, but don't). Not sure what the right tradeoff is here (how common > are ext3/FAT32 these days?). 1gb is an extreme case because it is likely to blow the filesystem write cache. A 5mb write is very likely to not stall our application due to happening entirely in fs cache. > > If we decide to fallocate a smaller size than the current max (5 MB), should we > keep track of how many bytes have been written, and fallocate another block > again when we exceed the initial size? I think that's reasonable in a followup. > > Slight tangent: do we want to open a bug for lazy creation of our CACHE_00x > files? Right now they all get created at once (in OpenBlockFiles), the first > time any cache entry is requested (which is usually pretty early during > startup), and at 20 MB each for three files, that could block cache access for > 1200 ms (or longer once we have more files for larger block sizes). Those files start out at 4mb each. OTOH, this > only happens when you launch for the very first time, or after a crash (or > cache version update, like FF4). Sounds good. We should certainly measure this. > > On ext4 and other new filesystems, fallocate is essentially free, so this is > all moot. I'm not sure how SetEndOfFile (the win32 equivalent) does on NTFS. Apparently SetEndOfFile delays zeroing until data is written(and maybe read?) from file. I agree we need to measure this more. I think adding a FunctionTimer to mozilla::fallocate is reasonable.

Comment on attachment 477471 [details] [diff] [review] patch v3 > I am worried about allocating 5mb for an entry that might fail to download due to possibility of wasting disk space. Ah--very good point. OK, let's land this with 1 MB. I've created bug 599065 for followup for >1MB files. > I think adding a FunctionTimer to mozilla::fallocate is reasonable. Taras: you want a separate bug for that, or should we just shove it into this patch?

(In reply to comment #12) > Taras: you want a separate bug for that, or should we just shove it into this > patch? If that's easy, pleas do.

This patch has been backed out: http://hg.mozilla.org/mozilla-central/rev/dcbc869144d4 http://hg.mozilla.org/mozilla-central/rev/b206601bf528 because of errors on Windows: http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1287660958.1287665344.6408.gz http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1287665218.1287667461.17678.gz http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1287665218.1287667478.17752.gz http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1287665218.1287667461.17677.gz http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1287665218.1287667461.17680.gz http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1287665218.1287667461.17679.gz http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1287665029.1287669262.27103.gz http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1287665217.1287667670.18648.gz Debug build + M{1,2,3,4,5,oth}, R. Fails in Opt too.

Not sure if it's worth mentioning since it was backed out, but the hourly build with this patch in it caused issues with loading the stylesheet on mozillazine.org. Looking at the .css file, it appears to cut off mid-style ("position: abs"). Some warnings were logged to the error console: Warning: Selector expected. Ruleset ignored due to bad selector. Source File: style.php?id=1&lang=en Line: 1 Warning: Error in parsing value for 'position'. Declaration dropped. Source File: style.php?id=1&lang=en Line: 1665 Warning: Unexpected end of file while searching for closing } of declaration block. Source File: style.php?id=1&lang=en Line: 1665 Reloading the page (Ctrl-R, Ctrl-Shift-R, F5) causes the page's stylesheet to load correctly, but it goes back to broken on the next page load.

(In reply to comment #17) > Some warnings were logged to the error console: > Warning: Selector expected. Ruleset ignored due to bad selector. > Source File: style.php?id=1&lang=en > Line: 1 This one is actually just bad CSS syntax on the website's end, not properly commenting out multi-line comments.

Comment on attachment 487915 [details] [diff] [review] fixed failures on Windows We should fix this in mozilla:fallocate, not in every caller of it. mozilla::fallocate shouldn't change the file pointer from the caller's point of view. We should have it store the current point in the file, and restore it to the same place after the seek that extends the file.

(In reply to comment #20) > Comment on attachment 487915 [details] [diff] [review] > fixed failures on Windows > > We should fix this in mozilla:fallocate, not in every caller of it. > > mozilla::fallocate shouldn't change the file pointer from the caller's point of > view. We should have it store the current point in the file, and restore it to > the same place after the seek that extends the file. Michal do you need help with implementing that fix?

I could get to it tomorrow or on Thursday. If you have free cycles, feel free to take it over.

Fixed in mozilla::fallocate() for XP_WIN and XP_UNIX. And pushed to try server.

Comment on attachment 492228 [details] [diff] [review] patch v5 Ah, thanks for catching the XP_UNIX case, too--I'd missed that. >diff --git a/xpcom/glue/FileUtils.cpp b/xpcom/glue/FileUtils.cpp >- if(-1 == ret){ >+ if(-1 == ret){ nit: add space between ) and {

Pushed to try server again because some tests failed in previous run.

Try server is green.

Pushed: http://hg.mozilla.org/mozilla-central/rev/59144b59cd11

This is still suspected of causing cache problems (see bug 617123 comment 3), so I've removed the cache's calls to fallocate for now: http://hg.mozilla.org/mozilla-central/rev/8d59a3285d92 Leaving this closed--work continues in 617123

Reopened, since sicking saw fit to close 617123. It's not clear if the backout in comment 28 actually fixed bug 617123. We should re-land this after beta8 and see if we get corrupted images again.

I think by now we need to not worry about this for Firefox 4, we'll get this in shortly thereafter.

(In reply to comment #30) > I think by now we need to not worry about this for Firefox 4, we'll get this in > shortly thereafter. I'm clearing the checkin-needed flag then so that somebody doesn't land this by mistake.

Is this not going in to Fx5?

This doesn't apply cleanly any more.

> This doesn't apply cleanly any more. Because the v6 patch wasn't backed out, only disabled. Re-enabled by fix for bug 617123.