streams: cache file position within AutoFile

sipa commented at 3:57 pm on September 12, 2024: member

Instead of relying on frequent ftell calls (which appear to cause a significant slowdown on some systems) in XOR-enabled AutoFiles, cache the file position within AutoFile itself.

DrahtBot commented at 3:57 pm on September 12, 2024: contributor

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	theStack, achow101, davidgumberg

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#29641 (scripted-diff: Use LogInfo over LogPrintf by maflcko)
#29307 (util: explicitly close all AutoFiles that have been written by vasild)
#28792 (Embed default ASMap as binary dump header file by fjahr)

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.

sipa force-pushed on Sep 12, 2024

DrahtBot added the label CI failed on Sep 12, 2024

davidgumberg commented at 8:45 pm on September 12, 2024: contributor

Nice! I am still running benchmarks, but early results indicate that this addresses #30833:

In my current run, up to block 240,000 v28.0rc1 bitcoind spent 967.67s writing undo data and your branch cherry picked onto the v28.x branch spents 1640.60s writing undo data, and like andrewtoth mentioned a lot of the speed regression as measured by wall clocks comes from AcceptBlock which doesn’t have any bench logs.

I’ll update with the wall clock times once my bench runs finish.

I think that there are also some calls to std::rewind: https://github.com/bitcoin/bitcoin/blob/cf0120ff024aa73a56f2975c832fda6aa8146dfa/src/test/streams_tests.cpp#L264

and std::fgetc: https://github.com/bitcoin/bitcoin/blob/cf0120ff024aa73a56f2975c832fda6aa8146dfa/src/node/utxo_snapshot.cpp#L76

that are problematic if we are caching file position.

Also, if it seems sensible to mitigate the risk of someone modifying the file position in the future without changing m_position, we could drop AutoFile::Get() entirely, e.g.: https://github.com/davidgumberg/bitcoin/commit/d238c46a44292d5ed81d703089b66be333a68083

sipa force-pushed on Sep 12, 2024

sipa commented at 9:43 pm on September 12, 2024: member

@davidgumberg Good points. I’ve made minimal changes in this PR to make sure no position-effecting operations remain, as we’re past the feature freeze. Extending the AutoFile interface so that AutoFile::Get is no longer needed can be done as a follow-up.

vostrnad commented at 11:58 pm on September 12, 2024: none

I’m happy to report that a quick benchmark of 4cfff4e58c6d806e4bc5a12386f84ff207c83419 cherry-picked onto v28.0rc1 shows that the slowdown is almost or entirely gone. I’ll do a more thorough benchmark later once the code is ACKed.

in src/streams.cpp:48 in 4cfff4e58c outdated

52+    } else {
53+        int64_t r{std::ftell(m_file)};
54+        if (r < 0) {
55+            throw std::ios_base::failure("AutoFile::seek: ftell failed");
56+        }
57+        m_position = r;

davidgumberg commented at 2:48 am on September 13, 2024:

Only commenting because I struggled with this for a bit:

This makes sense since the only present possibility for this else branch is that origin == SEEK_END, in which case the only way for fseek to have returned zero is if offset is a negative number indicating how far from the end of the file to seek to, and m_position = length_of_file + negative_offset

But, in order to get the length of the file you need to fseek(m_file, 0, SEEK_END) and ftell(m_file).

So, just ftell here is the best way to get the new m_position.

in src/streams.cpp:22 in 4cfff4e58c outdated

25-        const auto init_pos{std::ftell(m_file)};
26-        if (init_pos < 0) throw std::ios_base::failure("AutoFile::read: ftell failed");
27-        std::size_t ret{std::fread(dst.data(), 1, dst.size(), m_file)};
28-        util::Xor(dst.subspan(0, ret), m_xor, init_pos);
29-        return ret;
30+    size_t ret = std::fread(dst.data(), 1, dst.size(), m_file);

davidgumberg commented at 4:02 am on September 13, 2024:

Outside of the scope of this PR: I feel like this should probably handle a failed fread explicitly, even though the subspan call below means XOR won’t do anything bad here, and when this returns to AutoFile::read() or BufferedFile::fill() (it’s only two callers) the error gets handled. It seems kind of footgun-like to let this potentially dangerous file pointer get passed around and I’m not sure it’s worth BufferedFile being able to print it’s own name when throwing an exception.

laanwj commented at 6:06 am on September 13, 2024:

Handling i/o errors (eg partial reads/writes can be considered fatal for disk i/o) is always important, but yes even more-so now that the file pointer is cached and might get out of sync with the world.

in src/streams.cpp:11 in 4cfff4e58c outdated

 6@@ -7,18 +7,24 @@
 7 
 8 #include <array>
 9 
10+AutoFile::AutoFile(std::FILE* file, std::vector<std::byte> data_xor)
11+    : m_file{file}, m_xor{std::move(data_xor)}, m_position{0}

davidgumberg commented at 5:07 am on September 13, 2024:

clang-tidy seems upset about setting m_position{0} here instead of using C++11 “Default member initialization” in the class declaration (https://clang.llvm.org/extra/clang-tidy/checks/modernize/use-default-member-init.html)

sipa commented at 11:28 am on September 13, 2024:

Fixed.

in src/streams.cpp:15 in 4cfff4e58c outdated

 6@@ -7,18 +7,24 @@
 7 
 8 #include <array>
 9 
10+AutoFile::AutoFile(std::FILE* file, std::vector<std::byte> data_xor)
11+    : m_file{file}, m_xor{std::move(data_xor)}, m_position{0}
12+{
13+    if (!IsNull()) {
14+        auto pos{std::ftell(m_file)};
15+        if (pos >= 0) m_position = pos;

davidgumberg commented at 5:13 am on September 13, 2024:

nit: Handle ftell error

0        if (pos < 0) {
1            throw std::ios_base::failure("AutoFile::seek: ftell failed");
2        }
3        m_position = pos;

sipa commented at 11:05 am on September 13, 2024:

I tried that and it caused test failures. My reasoning is that the approach here is fine, because a failure to tell what position a FILE* is at means that further operations will fail too, anyway.

And there are also files for which fseek/ftell just don’t work (e.g. pipes/fifos, and /proc files). As long as no xoring is in use, this should be fine.

EDIT: see update below; I’ve made m_position an optional so it can represent “unknown position” instead.

davidgumberg commented at 8:42 pm on September 16, 2024:

That makes sense, I didn’t consider pipes/fifos.

I agree on this approach given that a bad m_position value is only an issue when XOR’ing, and your branch has checks for an unknown m_position before any XOR operations. And in general, it seems to me, ftell’s are followed by file operations that fail if the ftell fails for any reason other than the file being a pipe/fifo (ESPIPE).

I think ideally errno is checked here for values other than ESPIPE, but the current approach is good.

in src/streams.cpp:77 in 4cfff4e58c outdated

64@@ -60,6 +65,7 @@ void AutoFile::ignore(size_t nSize)
65             throw std::ios_base::failure(feof() ? "AutoFile::ignore: end of file" : "AutoFile::ignore: fread failed");
66         }
67         nSize -= nNow;
68+        m_position += nNow;
69     }
70 }

davidgumberg commented at 6:35 am on September 13, 2024:

Out of scope but I found while reviewing: Is there any reason why AutoFile::ignore(nSize) couldn’t be removed and AutoFile::seek(nSize, SEEK_CURR) used instead?

Maybe it would be okay to just leave ignore as an alias for seek if the semantics are sufficiently desirable.

sipa commented at 11:02 am on September 13, 2024:

I believe that when ignore is used, it’s usually for small amounts of data, which are likely already cached.

laanwj commented at 9:48 am on September 17, 2024:

Is there any reason why AutoFile::ignore(nSize) couldn’t be removed and AutoFile::seek(nSize, SEEK_CURR) used instead?

Keep in mind not all streams support seeking, so it would at the least be a conditional optimization. Agree it’s not worth it as long as the amounts skipped are small (generally smaller than the page size).

davidgumberg commented at 8:37 am on September 13, 2024: contributor

crACK https://github.com/bitcoin/bitcoin/pull/30884/commits/4cfff4e58c6d806e4bc5a12386f84ff207c83419

Awesome! My testing (n=2) indicates that this branch solves the Windows IBD regression reported in #30833.

In my bench runs your branch cherry picked on 28.x took 3% more wall clock time than v27.1 (3% is smaller than the fluctuations I observed during runs on the same branch), but v28rc1 took 112% more time to reach block 300k than 27.1.

I left a nit and some out-of-scope observations about some of the areas touched here most of which are best addressed in follow up PR’s. I tried to be as thorough as possible and could not think of a way for m_position to fall out of sync with the OS’s file pointer in the present branch. I think a future PR should definitely remove the AutoFile::Get() method to avoid users of the AutoFile interface from interacting with the raw FILE* and reintroducing calls that could desynchronize it.

Given the severity of a potential issue, it might be nice to add some fuzz/test checks that make sure that m_position desync doesn’t happen.

Benchmarks

I ran the same test (n=2) on three versions of Bitcoin Core: the v27.1 tag, the v28.0rc1 tag, and the 28.x branch with https://github.com/bitcoin/bitcoin/commit/8a7d8f7833e8e94b2152d0e67b0e67a1635d9894 ¹ cherry picked, all built the same way using mingw on a separate machine and without debug symbols stripped.

0git clean -dfx
1cd depends && make HOST=x86_64-w64-mingw32 -j $(nproc) && cd ../
2./autogen.sh && CONFIG_SITE=$PWD/depends/x86_64-w64-mingw32/share/config.site ./configure --prefix=/
3make -j $(nproc)

I ran all three tests on a Thinkpad T470 with an i5-6300U running Windows 10 Enterprise LTSC Build 17763. It has an SSD that my butt-dyno reports is pretty slow, which I suspect is important to reproduce #30833.

I used the following command for all six runs, clearing the datadir between each run:

0.\bitcoind.exe  -stopatheight=300000 -dbcache=450 -debug=bench -connect=fastlocalnode:8333 -datadir=C:\tmpdatadir -printtoconsole=0

Mean times to 300k (2 runs):

	Mean time
v27.1	0h 52min 21sec
v28rc1	1h 50min 47sec
28.x + #30833	0hr 53min 52sec

Here is the data from my individual runs, in your branch, write undo time are even smaller than v27.1

	Wall-clock to 300k blocks	Write undo time	BlockConnect Total
27.1 Run 1	0hr 53min 52sec	1,595.79s	2,164.82s
27.1 Run 2	0hr 50min 49sec	1,570.92s	2,146.43s
28rc1 Run 1	1hr 49min 27sec	2,707.95s	3,231.60s
28rc1 Run 2	1hr 52min 07sec	2,547.99s	3,091.75s
28.x + #30884 run 1	0hr 52min 34sec	1,138.34s	1,714.37s
28.x + #30884 run 2	0hr 55min 09sec	968.53s	1,535.17s

Out of the scope of this PR: One thing I find notable about the above data is that it seems that 28.x block connection times are down pretty significantly from 27.1, but wall clock IBD times are similar, indicating that there is a regression in some place other than ConnectBlock() at least on this machine with this setup, I don’t think it’s significant enough to block this release candidate, but I will investigate further.

Why the regression happened

I suspect that as suggested on IRC, on some systems, an ftell call that follows an fwrite triggers a flush. Apparently, one of the naive ways to implement ftell is as fseek(fp, 0, SEEK_CUR) but returning the fp offset. And an fseek after an fwrite results in a flush.² This is exactly the way glibc implemented this until in 2012 a patch was introduced which fixed this exact performance issue of extraneous flushes happening in loops where you ftell and fwrite many times by decoupling ftell calls from fseek:

glibc mailing list: [PATCH][BZ [#5298](/bitcoin-bitcoin/5298/)] Don't flush write buffer for ftell

 0Hi,
 1
 2The current implementation of ftell is basically equivalent to
 3fseek(fp, 0, SEEK_CUR). While this is not incorrect, it results in
 4inheritance of limitations of fseek, which is summarized in the
 5following comment in the source:
 6
 7  /* Flush unwritten characters.
 8     (This may do an unneeded write if we seek within the buffer.
 9     But to be able to switch to reading, we would need to set
10     egptr to ptr.  That can't be done in the current design,
11     which assumes file_ptr() is eGptr.  Anyway, since we probably
12     end up flushing when we close(), it doesn't make much difference.)
13     FIXME: simulate mem-papped files. */
14
15This is not needed for ftell since it does not need to set or
16modify buffer state, so this flush can be avoided. Attached patch
17computes current position for ftell (within the file_seekoff functions
18as a special case) without flushing the buffers when in write mode. I
19have used a modified version of the sample program in the bz (appended
20to this email) to check the improvement in performance in each call and
21the average reads as below on my Fedora 16 x86_64 core i5 laptop with
224GB RAM:
23
24Without patch:
25Total time: 9174470.000000 ns. AVG 1819.609282 ns per call
26
27With patch:
28Total time: 1047375.000000 ns. AVG 207.730067 ns per call
29
30I have verified that the change does not cause any regressions in the
31testsuite.
32
33Regards,
34Siddhesh

Here is the relevant bugzilla issue: https://sourceware.org/bugzilla/show_bug.cgi?id=5298 and a mirror of the commit: https://github.com/bminor/glibc/commit/adb26faefe47b7d34c941cbfc193ca7a5fde8e3f

I feel it’s likely that Windows libc either made the same choice, or some other design constraint forces windows to flush on ftell.

The reason ftell should act like fseek at all is because in the ballet that the 6 internal streambuffer pointers (eback, gptr, egptr, pbase, pptr, epptr) do as reads and writes happen, read pointer (gptr) != write pointer(pptr) != the file position offset, and especially after a write, pptr advances past egptr (which it seems in the case of file i/o is usually == gptr??) . So, the naive solution to this problem is to reuse fseek which, according to some C standards documents (https://pubs.opengroup.org/onlinepubs/007904975/functions/fseek.html),

The following is guaranteed as an effect of fseek():

0If the stream is writable and buffered data had not been written to the underlying file, fseek() shall cause the unwritten data to be written to the file and shall mark the st_ctime and st_mtime fields of the file for update.

So fseek brings all into happy sync, and now egptr == gptr == pptr.

But this is not necessary for ftell which does not need any updates to the streambuf’s internal state, and can basically just use do some pointer arithmetic from pptr, egptr, and the current offset to find the new offset without needing to flush anything to disk.

Information about the pointer arithmetic in this mailing list post: https://sourceware.org/legacy-ml/libc-alpha/2012-09/msg00631.html and relevant source code for calculating the offset for ftell: https://github.com/bminor/glibc/blob/adb26faefe47b7d34c941cbfc193ca7a5fde8e3f/libio/fileops.c#L1016-L1023 and relevant source code for the flush (_IO_OVERFLOW) that fseek` does to every buffer that arrives written and unflushed: https://github.com/bminor/glibc/blob/c9154cad66aa0b11ede62cc9190d3485c5ef6941/libio/genops.c#L189-L209

Not the most current branch but the follow-up rebase https://github.com/bitcoin/bitcoin/pull/30884/commits/4cfff4e58c6d806e4bc5a12386f84ff207c83419 makes trivial changes that should not affect these benchmarks. ↩︎
From IEEE Std 1003.1, 2004 Edition: If the stream is writable and buffered data had not been written to the underlying file, fseek() shall cause the unwritten data to be written to the file and shall mark the st_ctime and st_mtime fields of the file for update. ↩︎

Oaksta85 approved

sipa force-pushed on Sep 13, 2024

sipa commented at 11:33 am on September 13, 2024: member

I made a significant change here, making m_position an std::optional<int64_t>, which is std::nullopt when the position is unknown (due to ftell failing, and no successful fseek afterwards).

The position being unknown will permit normal operation, but throws an error on read/write when a xor key is set, or when invoking tell.

streams: cache file position within AutoFile e624a9bef1

sipa force-pushed on Sep 13, 2024

laanwj commented at 1:19 pm on September 13, 2024: member

I made a significant change here, making m_position an std::optional<int64_t>, which is std::nullopt when the position is unknown (due to ftell failing, and no successful fseek afterwards).

i first couldn’t think of a valid scenario where ftell would fail but we’d want to continue, but i guess this maks sense when reading a FIFO or device, when there isn’t really an offset.

fanquake added this to the milestone 28.0 on Sep 13, 2024

fanquake added the label Needs backport (28.x) on Sep 13, 2024

DrahtBot removed the label CI failed on Sep 13, 2024

achow101 commented at 4:13 pm on September 13, 2024: member

ACK e624a9bef16b6335fd119c10698352b59bf2930a

Extending the AutoFile interface so that AutoFile::Get is no longer needed can be done as a follow-up.

It does seem a bit fragile to still have AutoFile::Get. I would okay with backporting additional commits that remove it if they are not too complicated.

DrahtBot requested review from davidgumberg on Sep 13, 2024

streams: remove AutoFile::Get() entirely

Co-Authored-By: David Gumberg <davidzgumberg@gmail.com>

a240e150e8

sipa commented at 2:02 am on September 14, 2024: member

@achow101 I’ve added a commit that gets rid of AutoFile::Get entirely (based on @davidgumberg’s d238c46a44292d5ed81d703089b66be333a68083).

vostrnad commented at 5:52 pm on September 15, 2024: none

I’ve benchmarked commit e624a9bef16b6335fd119c10698352b59bf2930a, both in this branch and cherry-picked onto a few selected commits (fresh sync to block 400,000 from a local node, using default settings, all binaries self-built using Mingw-w64, around 10 runs for each version).

Summary: some slowdown due to #28052 definitely remains, but it’s under 5%.

version	result	note
fa7f7ac040a9467c307b20e77dc47c87d7377ded	2112 ± 17 seconds	First commit of #28052, and last commit before the regression.
fa7f7ac040a9467c307b20e77dc47c87d7377ded + e624a9bef16b6335fd119c10698352b59bf2930a	2116 ± 19 seconds	As above, but with this PR cherry-picked to verify it doesn’t on its own introduce any slowdown (it doesn’t).
fa895c72832f9555b52d5bb1dba1093f73de3136 + e624a9bef16b6335fd119c10698352b59bf2930a	2209 ± 17 seconds	Top commit of #28052 with this PR cherry-picked. This brings the slowdown introduced in #28052 to around 4-5%.
e624a9bef16b6335fd119c10698352b59bf2930a	2144 ± 12 seconds	This PR, not cherry-picked onto anything.

in src/node/utxo_snapshot.cpp:81 in e624a9bef1 outdated

77+    int64_t position = afile.tell();
78+    afile.seek(0, SEEK_END);
79+    if (position != afile.tell()) {
80         LogPrintf("[snapshot] warning: unexpected trailing data in %s\n", read_from_str);
81     } else if (std::ferror(afile.Get())) {
82         LogPrintf("[snapshot] warning: i/o error reading %s\n", read_from_str);

theStack commented at 11:08 pm on September 15, 2024:

Possible follow-up material: now that that the read operation above (std::fgetc) has been replaced by only fseek/ftell calls on the file object, I think this condition can’t be true anymore and hence this else-branch could be removed.

sipa commented at 11:31 am on September 19, 2024:

Done in #30927.

theStack approved

theStack commented at 11:16 pm on September 15, 2024: contributor

Code-review ACK a240e150e837b5a95ed19765a2e8b7c5b6013f35

Didn’t run any benchmarks to compare with master.

DrahtBot requested review from achow101 on Sep 15, 2024

achow101 commented at 4:01 pm on September 16, 2024: member

ACK a240e150e837b5a95ed19765a2e8b7c5b6013f35

in src/streams.cpp:116 in a240e150e8

111+    return ferror(m_file);
112+}
113+
114+bool AutoFile::Truncate(unsigned size)
115+{
116+    return ::TruncateFile(m_file, size);

davidgumberg commented at 9:45 pm on September 16, 2024:

Note: It is okay to do this without worrying about m_position because ftruncate does not modify the position of the file seek pointer. (https://pubs.opengroup.org/onlinepubs/007904975/functions/ftruncate.html)

DrahtBot requested review from davidgumberg on Sep 16, 2024

davidgumberg commented at 10:05 pm on September 16, 2024: contributor

untested reACK https://github.com/bitcoin/bitcoin/pull/30884/commits/a240e150e837b5a95ed19765a2e8b7c5b6013f35 Changes since https://github.com/bitcoin/bitcoin/commit/4cfff4e58c6d806e4bc5a12386f84ff207c83419 should not change any behavior, they only make the AutoFile interfaces safer by:

Ensuring we only perform XOR operations or AutoFile::tell() if the file has a sensible seek pointer; e.g. if you try to ftell a pipe: ftell(stdout) -1 will be returned, and errno will be set to ESPIPE, even though it might still be valid to read and write from the “file”.
Removing the AutoFile::Get() interface which allows AutoFile users to access the raw FILE pointer and potentially modify the OS file position without modifying m_position.

achow101 merged this on Sep 17, 2024

achow101 closed this on Sep 17, 2024

achow101 referenced this in commit 8229e98116 on Sep 17, 2024

achow101 referenced this in commit ff95cb31da on Sep 17, 2024

achow101 commented at 3:10 am on September 17, 2024: member

Backported in #30827

laanwj commented at 9:55 am on September 17, 2024: member

Code review ACK a240e150e837b5a95ed19765a2e8b7c5b6013f35

fanquake removed the label Needs backport (28.x) on Sep 17, 2024

fanquake referenced this in commit 1147e72953 on Sep 17, 2024

in src/node/blockstorage.cpp:689 in e624a9bef1 outdated

682@@ -683,7 +683,7 @@ bool BlockManager::UndoWriteToDisk(const CBlockUndo& blockundo, FlatFilePos& pos
683     fileout << GetParams().MessageStart() << nSize;
684 
685     // Write undo data
686-    long fileOutPos = ftell(fileout.Get());
687+    long fileOutPos = fileout.tell();
688     if (fileOutPos < 0) {
689         LogError("%s: ftell failed\n", __func__);
690         return false;

maflcko commented at 9:44 am on September 19, 2024:

Isn’t this dead code now? Previously it was required, because std::ftell could fail and return -1L.

However, now that AutoFile::tell() is used, which does not return an error value (but throws on error), this code can never execute.

A negative value here could only happen if seeking prior to the start of a file was possible, which I don’t think it is. Also, the AutoFile constructor already assumes this isn’t possible.

In any case, the prior line writes to the file, which isn’t possible/supported for negative absolute positions, so this must be dead code in this function either way.

sipa commented at 11:30 am on September 19, 2024:

Done in #30927.

in src/node/blockstorage.cpp:987 in e624a9bef1 outdated

980@@ -981,7 +981,7 @@ bool BlockManager::WriteBlockToDisk(const CBlock& block, FlatFilePos& pos) const
981     fileout << GetParams().MessageStart() << nSize;
982 
983     // Write block
984-    long fileOutPos = ftell(fileout.Get());
985+    long fileOutPos = fileout.tell();
986     if (fileOutPos < 0) {
987         LogError("%s: ftell failed\n", __func__);
988         return false;

maflcko commented at 9:45 am on September 19, 2024:

Same

sipa commented at 11:31 am on September 19, 2024:

Done in #30927.

maflcko commented at 11:17 am on September 19, 2024: member

review ACK a240e150e837b5a95ed19765a2e8b7c5b6013f35 🍽

Signature:

0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: review ACK a240e150e837b5a95ed19765a2e8b7c5b6013f35 🍽
3E+QumBxAsjs6onMxIQw5fqCgJWho8UUHBtPokAtkGM+wtysIawMw5OCTX9k7LQyI6+s2neQYq1Xh/+7Ifg6bCw==

streams: cache file position within AutoFile #30884

Code Coverage

Reviews

Conflicts

Benchmarks

Why the regression happened