Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

add Windows specific optimization to pre-alloc block/undo files #2098

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
3 participants

Diapolo commented Dec 12, 2012

  • add AllocateFileRangeWin(), which is used to pre-allocate block/undo files
    as a single contignous chunk on disk, so these files are not fragmented
    (current master has 95 - 409 fragments for such files, this patch
    reduces all those to 1 fragment)
  • add GetBlockFile() and GetUndoFile() helper functions, which are a
    wrapper for GetDiskFile(), which caches the last used file (separate
    cache for the last block and undo file)

I guess the helper functions could be used in other places of the code as well. I have another pull in the pipe, which makes CAutoFile based on an std::fstream and I use these helper functions there, too.

Todo:

  • perhaps there is no need for another function name even, so AllocateFileRangeWin() could be changed to AllocateFileRange()
  • as I didn't know how big undo files can grow (no MAX_UNDOFILE_SIZE from @sipa ^^), I used the same size for them, as for the block files (MAX_BLOCKFILE_SIZE)
add Windows specific optimization to pre-alloc block/undo files
- add AllocateFileRangeWin(), which is used to pre-allocate block/undo files
  as a single contignous chunk on disk, so these files are not fragmented
  (current master has 95 - 409 fragments for such files, this patch
  reduces all those to 1 fragment)

- add GetBlockFile() and GetUndoFile() helper functions, which are a
  wrapper for GetDiskFile(), which caches the last used file (separate
  cache for the last block and undo file)
Owner

sipa commented Dec 12, 2012

Why do you keep insisting on having all block files in a single continuous chunk? I implemented pre-allocating specifically to address the excessive fragmentation the previous mechanism seemed to cause on some platform, but it's unreasonable to want every file to only be one chunk.

I'll consider this if you can show benchmark results that this helps. PS: block and undo files are almost never accessed in the new code, contrary to 0.7 which needed them all the time for validation.

A patch to just implement AllocateFileRange specifically for Windows is no problem at all.

Diapolo commented Dec 12, 2012

Because I have the code and it's working fine, as it does, what it is intended for. Why is something unreasonable, if it's possible to do via some rather simple OS-calls?

I added some code to print to the log when ReadFromDisk() and WriteToDisk() are called and at least ReadFromDisk() seems rather sequential during startup (talking about Verifying last 2500 blocks at level 1 and Rescanning last 199 blocks...).

Even if it is not a big increase for the Bitcoin client, it helps to lower overall fragmentation of users disks.

Edit: Can you tell me how big undo files can grow at max?

Contributor

gavinandresen commented Dec 12, 2012

NACK. 88 more lines of code to maintain is a greater cost than the potential benefit here.

Owner

sipa commented Dec 12, 2012

First of all, dealing with fragmentation is a reasonable request. It's not Windows-specific by the way - if you don't allocate a file in one go, it will lead to fragmentation on just about every system and every filesystem. Pre-allocation is the solution to this, and it's already implemented. If Windows has a better way for pre-allocating than writing zeroes, sure provide an optimized version for that specific function in util.cpp - that's what it's there for.

But please:

  • Don't put platform-specific stuff in the block management code because you fear 100 fragments will hurt you. If it does, we need to increase the pre-allocation size perhaps.
  • Don't allocate entire files without knowing you'll need it. That's just wasted space. For the block files this is limited (as they grow anyway, so you're on average only 64 MiB above needed), but for undo files it's simply not known how large they'll grow (they contain the undo data corresponding to the block with the same number). Making them any specific size will either be a permanent waste, or not enough.

Diapolo commented Dec 12, 2012

@sipa At least you try to clarify your position... though I don't agree on the 100 fragments part. The undofile size is a reasonable comment, but for blockfiles a to be filled 128MB file is clearly not wasted space. I'll keep using this with my local build, but guess I won't create a pull for that CAutoFile thing as I dislike Gavins dictatorial nature on patches, I put quite some time in...

Owner

sipa commented Dec 13, 2012

@Diapolo my suggestion:

  1. implement AllocateFileRange for Windows (just inside the function body, using macros) that doesn't try to do more than that function intends.
  2. If, after benchmarking with 1) you can still observe a noticable slowdown, we can increase the chunk size for Windows - or make it configurable in the first place.

That should have exactly the same effect as your current code, in something like 10 lines of code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment