Do not fragment the hell out of CACHE__00[1-3]__

RESOLVED FIXED

Status

()

defect
RESOLVED FIXED
9 years ago
9 years ago

People

(Reporter: taras.mozilla, Assigned: taras.mozilla)

Tracking

(Blocks 1 bug)

unspecified
x86
Linux
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 betaN+)

Details

Attachments

(2 attachments, 6 obsolete attachments)

No description provided.
Blocks: 592522
Posted patch proof of concept (obsolete) — Splinter Review
This starts the cache files at 1mb. Then grows them by doubling the file size.
Posted patch no frags (obsolete) — Splinter Review
This works on Linux/Windows and likely on osx(will test on try)
Attachment #471013 - Attachment is obsolete: true
Attachment #471214 - Flags: review?(byronm)
Attachment #471214 - Flags: superreview?(benjamin)
Comment on attachment 471214 [details] [diff] [review]
no frags

Mac doesn't have posix_fallocate, so the ifdef is definitely incorrect. It's not entirely clear to me whether we expect this method to be unimplemented in normal cases, and whether it's an advisory call or actually changes the file size in important ways. Can you explain (preferably in the doccomment).
Attachment #471214 - Flags: superreview?(benjamin) → superreview-
Added HAVE_POSIX_FALLOCATE(which turns out to be needed for equivalent functionality in sqlite http://mxr.mozilla.org/mozilla-central/source/db/sqlite3/src/sqlite3.c#25687) + fallback.
Attachment #471214 - Attachment is obsolete: true
Attachment #471320 - Flags: review?(benjamin)
Attachment #471214 - Flags: review?(byronm)
Attachment #471320 - Flags: review?(benjamin)
Blocks: 593198
This passes try.
Attachment #471320 - Attachment is obsolete: true
Attachment #471714 - Flags: review?(benjamin)
Blocks: http_cache
Found http://lists.apple.com/archives/darwin-dev/2007/Dec/msg00040.html which seems to show to how to do this on osx so I can avoid the glibc-style workaround and do this properly on all supported platforms.
Comment on attachment 471714 [details] [diff] [review]
no frags with a fallback for posix_fallocate

r=me on the XPCOM bits. I did not review the necko bits.
Attachment #471714 - Flags: review?(benjamin) → review+
Comment on attachment 471714 [details] [diff] [review]
no frags with a fallback for posix_fallocate

Can you review the network bits? I'd like to land this asap. I found a profile where disk io is magnitudes slower than the network.
Attachment #471714 - Flags: review?(jduell.mcbugs)
Jason,
I've been waiting for a review for a week, can you give me an ETA? I'd like to land this asap.
Posted patch magic mac fallocate dance (obsolete) — Splinter Review
This is an addon patch that implement low-fragmentation allocation on mac.

Trivia: Mac is special in some ways, it allows one to demand a continuous chunk of disk... Which is cool. This allows one to also detect fragmentation. On the other hand ftruncate seems to actually write out zeros to disk instead of doing clever things on fs metadata level.
Attachment #474233 - Flags: review?(benjamin)
Blocking per discussion with Taras, high return fix here.
blocking2.0: --- → betaN+
Comment on attachment 471714 [details] [diff] [review]
no frags with a fallback for posix_fallocate

>+bool
>+nsDiskCacheBlockFile::Write(PRInt32 offset, const void *buf, PRInt32 amount)

>+        if (upTo > maxPreallocate) {
>+            mFileSize = upTo;
>+        } else {
>+            // Grow quickly between 1MB to 20MB
>+            if (mFileSize)
>+                while(mFileSize < upTo)
>+                    mFileSize *= 2;
>+        }

So we're doubling file size until we hit 20 MB, then just adding blocks incrementally?   It it worth doing continuing to do large fallocates after >20MB, esp on non-OSX?  (maybe not always doubling, but at least grabbing big fixed sized amounts).

Either way, +r.
Attachment #471714 - Flags: review?(jduell.mcbugs) → review+
Attachment #474233 - Flags: review?(benjamin) → review+
Carrying over review from bsmedberg/jduel.
Attachment #471714 - Attachment is obsolete: true
Attachment #474233 - Attachment is obsolete: true
Attachment #475666 - Flags: review+
Keywords: checkin-needed
Comment on attachment 475666 [details] [diff] [review]
no frags with a fallback for posix_fallocate/fcntl

This patch doesn't have the "start at 4mb, then grow by 4mb after we hit 20 mb" logic we talked about.  Another patch coming?
Attachment #475666 - Attachment is obsolete: true
Attachment #475681 - Flags: review+
Comment on attachment 475681 [details] [diff] [review]
no frags with a fallback for posix_fallocate/fcntl(for real)

So, just to be clear, this patch has the cache file grow in this series:  4, 8, 16, 20, 24, 28, ...

Almost doesn't seem worth the fuss to have the doubling logic in place when it's only used once.  How about 5 MB units, so 5, 10, 20, 25, 30, ...? 

I'm still not completely clear on the significance of the 20 MB filesize on OSX. What do we lose after files get bigger than that?
(In reply to comment #16)
> Comment on attachment 475681 [details] [diff] [review]
> no frags with a fallback for posix_fallocate/fcntl(for real)
> 
> So, just to be clear, this patch has the cache file grow in this series:  4, 8,
> 16, 20, 24, 28, ...
> 
> Almost doesn't seem worth the fuss to have the doubling logic in place when
> it's only used once.  How about 5 MB units, so 5, 10, 20, 25, 30, ...? 
> 
> I'm still not completely clear on the significance of the 20 MB filesize on
> OSX. What do we lose after files get bigger than that?

Can get rid of the doubling logic if you like. So my growing approach minimizes fragmentation, but the file is still likely to be fragmented at least once per growth cycle. ie with your 5mb increment, there would be at least 4 fragments
for a 20mb file. So as long as the file is <20mb OSX will turn that into a single fragment.
http://hg.mozilla.org/mozilla-central/rev/26e2971eeec9
Status: NEW → RESOLVED
Closed: 9 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Attachment #476041 - Flags: review? → review?(tglek)
Attachment #476041 - Flags: review?(tglek) → review+
You need to log in before you can comment on or make changes to this bug.