Skip to content

[libc++] Optimize fstream::read #165223

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Nov 7, 2025
+37 −6
Merged

[libc++] Optimize fstream::read #165223

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions libcxx/docs/ReleaseNotes/22.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -64,8 +64,8 @@ Improvements and New Features
by up to 2.5x
- The performance of ``erase(iterator, iterator)`` in the unordered containers has been improved by up to 1.9x
- The performance of ``map::insert_or_assign`` has been improved by up to 2x
- ``ofstream::write`` has been optimized to pass through large strings to system calls directly instead of copying them
in chunks into a buffer.
- ``ofstream::write`` and ``ifstream::read`` have been optimized to pass through large reads and writes to system calls
directly instead of copying them in chunks.
- Multiple internal types have been refactored to use ``[[no_unique_address]]``, resulting in faster compile times and
reduced debug information.

Expand Down
13 changes: 13 additions & 0 deletions libcxx/include/fstream
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -308,6 +308,19 @@ protected:
return basic_streambuf<_CharT, _Traits>::xsputn(__str, __len);
}

_LIBCPP_HIDE_FROM_ABI_VIRTUAL streamsize xsgetn(char_type* __str, streamsize __len) override {
if (__always_noconv_) {
const streamsize __n = std::min(this->egptr() - this->gptr(), __len);
if (__n != 0) {
traits_type::copy(__str, this->gptr(), __n);
this->__gbump_ptrdiff(__n);
}
if (__len - __n >= this->egptr() - this->eback())
return std::fread(__str + __n, sizeof(char_type), __len - __n, __file_);
Copy link
Member

@jsji jsji Nov 9, 2025

Choose a reason for hiding this comment

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

We are seeing regressions with this commit. Applications failing with message like:
InvalidWordCount: WordCount exceeds remaining input stream size: expected size = 47564 bytes, remaining size = 544 bytes

Should we return total number of characters read not only the number of characters read by fread here?
Also should we return __n; after the if?

Copy link
Contributor

@evodius96 evodius96 Nov 10, 2025

Choose a reason for hiding this comment

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

I am also seeing downstream failure in ACE C++ test suite Cxx11/27/9/1/5/t_xsgetn_1.C where it appears application is accessing invalid memory.

Copy link
Contributor

@Sterling-Augustine Sterling-Augustine Nov 10, 2025

Choose a reason for hiding this comment

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

I am also seeing failures with this commit. Shall we revert?

Copy link
Member

@jsji jsji Nov 11, 2025

Choose a reason for hiding this comment

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

@philnik777 Can you have a look? Thanks.

Copy link
Contributor

@michaelrj-google michaelrj-google Nov 12, 2025

Choose a reason for hiding this comment

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

I think I've got a patch that fixes the issue:

diff --git a/libcxx/include/fstream b/libcxx/include/fstream
index b07ca636094a..e97d378ae6ce 100644
--- a/libcxx/include/fstream
+++ b/libcxx/include/fstream
@@ -315,8 +315,11 @@ protected:
         traits_type::copy(__str, this->gptr(), __n);
         this->__gbump_ptrdiff(__n);
       }
-      if (__len - __n >= this->egptr() - this->eback())
-        return std::fread(__str + __n, sizeof(char_type), __len - __n, __file_);
+      const streamsize __remainder = __len - __n;
+
+      if (__remainder >= 0)
+        return std::fread(__str + __n, sizeof(char_type), __remainder, __file_) + __n;
+      return __n;
     }
     return basic_streambuf<_CharT, _Traits>::xsgetn(__str, __len);
   }

I'm planning to put up a PR tomorrow once I get a test written, but I won't be upset if someone beats me to it.

Copy link
Contributor

@Sterling-Augustine Sterling-Augustine Nov 12, 2025

Choose a reason for hiding this comment

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

Unfortunately, coming up with a test has been difficult. The fix PR, which works for our unreduced example, is in #167779

}
return basic_streambuf<_CharT, _Traits>::xsgetn(__str, __len);
}

private:
char* __extbuf_;
const char* __extbufnext_;
Expand Down
22 changes: 20 additions & 2 deletions ...est/benchmarks/streams/ofstream.bench.cpp → ...test/benchmarks/streams/fstream.bench.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@

#include <benchmark/benchmark.h>

static void bm_write(benchmark::State& state) {
static void bm_ofstream_write(benchmark::State& state) {
std::vector<char> buffer;
buffer.resize(16384);

Expand All @@ -20,6 +20,24 @@ static void bm_write(benchmark::State& state) {
for (auto _ : state)
stream.write(buffer.data(), buffer.size());
}
BENCHMARK(bm_write);
BENCHMARK(bm_ofstream_write);

static void bm_ifstream_read(benchmark::State& state) {
std::vector<char> buffer;
buffer.resize(16384);

std::ofstream gen_testfile("testfile");
gen_testfile.write(buffer.data(), buffer.size());

std::ifstream stream("testfile");
assert(stream);

for (auto _ : state) {
stream.read(buffer.data(), buffer.size());
benchmark::DoNotOptimize(buffer);
stream.seekg(0);
}
}
BENCHMARK(bm_ifstream_read);

BENCHMARK_MAIN();
2 changes: 1 addition & 1 deletion libcxx/test/libcxx/input.output/file.streams/fstreams/filebuf/traits_mismatch.verify.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,4 +19,4 @@

std::basic_filebuf<char, std::char_traits<wchar_t> > f;
// expected-error-re@*:* {{static assertion failed{{.*}}traits_type::char_type must be the same type as CharT}}
// expected-error@*:* 10 {{only virtual member functions can be marked 'override'}}
// expected-error@*:* 11 {{only virtual member functions can be marked 'override'}}
2 changes: 1 addition & 1 deletion libcxx/test/libcxx/input.output/file.streams/fstreams/traits_mismatch.verify.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ std::basic_fstream<char, std::char_traits<wchar_t> > f;
// expected-error-re@*:* {{static assertion failed{{.*}}traits_type::char_type must be the same type as CharT}}
// expected-error-re@*:* {{static assertion failed{{.*}}traits_type::char_type must be the same type as CharT}}

// expected-error@*:* 12 {{only virtual member functions can be marked 'override'}}
// expected-error@*:* 13 {{only virtual member functions can be marked 'override'}}

// FIXME: As of commit r324062 Clang incorrectly generates a diagnostic about mismatching
// exception specifications for types which are already invalid for one reason or another.
Expand Down
Loading