Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Try to use posix_fadvise with CBufferedFile #14485

Closed
wants to merge 1 commit into from

Conversation

luke-jr
Copy link
Member

@luke-jr luke-jr commented Oct 15, 2018

Resurrecting #12491, since it seemed fine and desirable...

@meshcollider
Copy link
Contributor

Concept ACK, would be good to hear from @eklitzke why the original was closed though

src/util.h Outdated
//! Close a file and return the result of fclose(). On POSIX systems that
//! support it, advise the kernel to remove the file contents from the page
//! cache (which can help on memory-constrained systems).
int CloseAndDiscard(FILE *file);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"discard" sounds to me as if writes to the file will be discarded, too
maybe CloseAndUncache?

@laanwj
Copy link
Member

laanwj commented Oct 18, 2018

Concept ACK though I'd really like to see benchmarks for changes like this, to know if this has any impact in practice worth making the risk/complication of the change.

@jgarzik
Copy link
Contributor

jgarzik commented Oct 18, 2018

Probably worthing having -- rough concept ACK -- however:

  • The double-lseek will be slower than fstat() on some networked or detachable filesystems.

  • If close-and-uncache is used on just-written-to files, this action is a no-op unless a sync (fdatasync etc.) preceded it.

  • Definitely worth benchmarking on multiple filesystems to see the positive or negative value of this change.

@luke-jr
Copy link
Member Author

luke-jr commented Feb 11, 2019

Rebased and addressed @laanwj's function rename request.

src/util/system.cpp Outdated Show resolved Hide resolved
@DrahtBot
Copy link
Contributor

DrahtBot commented Mar 23, 2019

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #26705 (clang-tidy: Fix modernize-use-default-member-init in headers and force to check all headers by hebasto)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

@DrahtBot
Copy link
Contributor

There hasn't been much activity lately and the patch still needs rebase, so I am closing this for now. Please let me know when you want to continue working on this, so the pull request can be re-opened.

@luke-jr
Copy link
Member Author

luke-jr commented Oct 13, 2019

Rebased

Copy link
Contributor

@vasild vasild left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My gut feel tells me that this is good, but experience tells me that it needs some performance testing to demonstrate that it really brings a boost. Sometimes such changes have surprisingly adverse effects or no effects and end up being just code clutter.

src/util/system.cpp Outdated Show resolved Hide resolved
src/util/system.cpp Outdated Show resolved Hide resolved
src/util/system.cpp Show resolved Hide resolved
src/util/system.cpp Show resolved Hide resolved
@luke-jr
Copy link
Member Author

luke-jr commented May 7, 2020

Rebased & comments addressed

@practicalswift
Copy link
Contributor

Adding to the three previous requests for benchmarks:

Do we have any benchmarks showing the practical gains in swiftness? :)

This primarily affects blocks when bitcoin is launched with -reindex, as
that causes the block files to be loaded as CBufferedFile objects one at
a time as the reindex progresses.

Co-Authored-By: Luke Dashjr <luke-jr+git@utopios.org>
@aureleoules
Copy link
Member

aureleoules commented Nov 4, 2022

This change seems to slow down IBD.
I benchmarked by syncing a fresh node both on master and this PR (rebased on master) until block height=495000 with -prune=550 and -connect=localhost (syncing from existing local full node).
Master took 5h27mins and this PR took 5h40mins.

#14485 (comment)

@vasild
Copy link
Contributor

vasild commented Nov 7, 2022

This is about 4% difference. Could it be within noise/variance? I mean if you run 3 times master and it takes e.g. 5h27min, 5h03min, 5h42min then you know the result from the PR is within noise. I think that if this makes some impact (positive or negative) it should be observable on smaller tests too, e.g. something that runs 5 or 10 minutes. Then you can repeat a few times and see how much is the variance for both master and the PR.

Is such a workload disk bound? If it is, then does the full node (the source of the sync) compete for disk resources too (that would slow down the syncing node and bring noise)? Was the full node also running the PR?

@aureleoules
Copy link
Member

aureleoules commented Nov 7, 2022

@vasild you're right, sorry about my last benchmark. I didn't lock the CPU frequency which probably caused a lot of noise.

I re-ran some benchmarks with CPU frequency locked. I rebased the PR locally on master.
The local node used to sync the benchmarked nodes runs on this PR.

pr: -prune=10000 -stopatheight=400000
Command being timed: "./bitcoind_pr -datadir=/tmp/btc -prune=10000 -stopatheight=400000 -connect=localhost -server=0 -printtoconsole=0"
        User time (seconds): 6765.53
        System time (seconds): 707.99
        Percent of CPU this job got: 137%
        Elapsed (wall clock) time (h:mm:ss or m:ss): 1:30:34
        Average shared text size (kbytes): 0
        Average unshared data size (kbytes): 0
        Average stack size (kbytes): 0
        Average total size (kbytes): 0
        Maximum resident set size (kbytes): 2742480
        Average resident set size (kbytes): 0
        Major (requiring I/O) page faults: 1858
        Minor (reclaiming a frame) page faults: 2802209
        Voluntary context switches: 19966530
        Involuntary context switches: 15899
        Swaps: 0
        File system inputs: 473456
        File system outputs: 333039544
        Socket messages sent: 0
        Socket messages received: 0
        Signals delivered: 0
        Page size (bytes): 4096
        Exit status: 0
master: -prune=10000 -stopatheight=400000
Command being timed: "./bitcoin/src/bitcoind -datadir=/tmp/btc -prune=10000 -stopatheight=400000 -connect=localhost -server=0 -printtoconsole=0"
        User time (seconds): 6944.92
        System time (seconds): 707.66
        Percent of CPU this job got: 137%
        Elapsed (wall clock) time (h:mm:ss or m:ss): 1:32:33
        Average shared text size (kbytes): 0
        Average unshared data size (kbytes): 0
        Average stack size (kbytes): 0
        Average total size (kbytes): 0
        Maximum resident set size (kbytes): 2733784
        Average resident set size (kbytes): 0
        Major (requiring I/O) page faults: 2161
        Minor (reclaiming a frame) page faults: 2674124
        Voluntary context switches: 20103862
        Involuntary context switches: 18039
        Swaps: 0
        File system inputs: 536328
        File system outputs: 334433416
        Socket messages sent: 0
        Socket messages received: 0
        Signals delivered: 0
        Page size (bytes): 4096
        Exit status: 0

The results seem to be similar, so ACK 3f2c08b.

@DrahtBot
Copy link
Contributor

DrahtBot commented Feb 1, 2023

🐙 This pull request conflicts with the target branch and needs rebase.

@achow101 achow101 requested a review from jamesob April 25, 2023 15:25
@DrahtBot
Copy link
Contributor

There hasn't been much activity lately and the patch still needs rebase. What is the status here?

  • Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
  • Is it no longer relevant? ➡️ Please close.
  • Did the author lose interest or time to work on this? ➡️ Please close it and mark it 'Up for grabs' with the label, so that it can be picked up in the future.

@achow101
Copy link
Member

This PR does not seem to have conceptual support. Please leave a comment if you would like this to be reopened.

@achow101 achow101 closed this Sep 20, 2023
luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Jun 13, 2024
This primarily affects blocks when bitcoin is launched with -reindex, as
that causes the block files to be loaded as CBufferedFile objects one at
a time as the reindex progresses.

Co-Authored-By: Luke Dashjr <luke-jr+git@utopios.org>

Github-Pull: bitcoin#14485
Rebased-From: 289e88b
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet