Closed Bug 592422 Opened 10 years ago Closed 9 years ago

preallocate individual (non-block) cache files

Categories

(Core :: Networking: Cache, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla5
Tracking Status
blocking2.0 --- .x+

People

(Reporter: taras.mozilla, Assigned: michal)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 6 obsolete files)

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.
Blocks: http_cache
Should be a one or two liner, could have a decent perf payoff, esp for startup.
blocking2.0: --- → ?
Summary: preallocate files for downloads → preallocate individual (non-block) cache files
Michal, can you write up this patch?
Assignee: nobody → michal.novotny
blocking2.0: ? → betaN+
Attached patch fix (obsolete) — Splinter Review
Attachment #477117 - Flags: review?(jduell.mcbugs)
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.
Attachment #477117 - Flags: review?(jduell.mcbugs) → review-
(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...
Attached patch patch v2 (obsolete) — Splinter Review
I've chosen 1MB. Feel free to suggest different limit.
Attachment #477117 - Attachment is obsolete: true
Attachment #477326 - Flags: review?(jduell.mcbugs)
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));
Attachment #477326 - Flags: review?(jduell.mcbugs) → review-
Attached patch patch v3 (obsolete) — Splinter Review
Attachment #477326 - Attachment is obsolete: true
Attachment #477471 - Flags: review?(jduell.mcbugs)
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.
Blocks: 599065
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?
Attachment #477471 - Flags: review?(jduell.mcbugs) → review+
(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.
Attached patch add timer to fallocate (obsolete) — Splinter Review
Attachment #477471 - Attachment is obsolete: true
Attachment #478032 - Flags: review?(tglek)
Attachment #478032 - Flags: review?(tglek) → review+
Keywords: checkin-needed
http://hg.mozilla.org/mozilla-central/rev/e72707266b4a
Status: NEW → RESOLVED
Closed: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.0b8
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.
Attached patch fixed failures on Windows (obsolete) — Splinter Review
Attachment #478032 - Attachment is obsolete: true
Attachment #487915 - Flags: review?(jduell.mcbugs)
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.
Attachment #487915 - Flags: review?(jduell.mcbugs) → review-
(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.
Attached patch patch v5 (obsolete) — Splinter Review
Fixed in mozilla::fallocate() for XP_WIN and XP_UNIX. And pushed to try server.
Attachment #487915 - Attachment is obsolete: true
Attachment #492228 - Flags: review?(jduell.mcbugs)
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 {
Attachment #492228 - Flags: review?(jduell.mcbugs) → review+
Attached patch patch v6Splinter Review
Pushed to try server again because some tests failed in previous run.
Attachment #492228 - Attachment is obsolete: true
Try server is green.
Keywords: checkin-needed
Pushed:
http://hg.mozilla.org/mozilla-central/rev/59144b59cd11
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Keywords: checkin-needed
OS: Linux → All
Hardware: x86 → All
Resolution: --- → FIXED
Version: unspecified → Trunk
Depends on: 615861
Depends on: 617123
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.
Status: RESOLVED → REOPENED
Keywords: checkin-needed
Resolution: FIXED → ---
I think by now we need to not worry about this for Firefox 4, we'll get this in shortly thereafter.
blocking2.0: betaN+ → .x
(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.
Keywords: checkin-needed
Is this not going in to Fx5?
Keywords: checkin-needed
Target Milestone: mozilla2.0b8 → ---
This doesn't apply cleanly any more.
Keywords: checkin-needed
> This doesn't apply cleanly any more.

Because the v6 patch wasn't backed out, only disabled.  Re-enabled by fix for bug 617123.
Status: REOPENED → RESOLVED
Closed: 9 years ago9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla2.2
You need to log in before you can comment on or make changes to this bug.