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.

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

Jump to bottom

Performance Tuning of std::filebuf #2481

Closed
wants to merge 1 commit into from
Closed

Performance Tuning of std::filebuf #2481

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
31 changes: 22 additions & 9 deletions stl/inc/fstream
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -211,6 +211,9 @@ public:

void swap(basic_filebuf& _Right) {
if (this != _STD addressof(_Right)) {
auto _Binary_sav = _Binary;
_Binary = _Right._Binary;
_Right._Binary = _Binary_sav;
Comment on lines +214 to +216
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use clang-format

FILE* _Myfile_sav = _Myfile;
const _Cvt* _Pcvt_sav = _Pcvt;
typename _Traits::state_type _State_sav = _State;
Expand Down Expand Up @@ -281,6 +284,7 @@ public:
}

basic_filebuf* open(const char* _Filename, ios_base::openmode _Mode, int _Prot = ios_base::_Default_open_prot) {
this->_Binary = _Mode & ios::binary;
Copy link
Contributor

Choose a reason for hiding this comment

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

We use this-> only when it is required to find template base member. Occurs below.

// _Prot is an extension
if (_Myfile) {
return nullptr;
Expand Down Expand Up @@ -308,6 +312,7 @@ public:
#endif // _HAS_OLD_IOSTREAMS_MEMBERS

basic_filebuf* open(const wchar_t* _Filename, ios_base::openmode _Mode, int _Prot = ios_base::_Default_open_prot) {
this->_Binary = _Mode & ios::binary;
// in standard as const std::filesystem::path::value_type *; _Prot is an extension
if (_Myfile) {
return nullptr;
Expand Down Expand Up @@ -356,6 +361,7 @@ public:
#ifdef _NATIVE_WCHAR_T_DEFINED
basic_filebuf* open(
const unsigned short* _Filename, ios_base::openmode _Mode, int _Prot = ios_base::_Default_open_prot) {
this->_Binary = _Mode & ios::binary;
// in standard as const std::filesystem::path::value_type *; _Prot is an extension
if (_Myfile) {
return nullptr;
Expand Down Expand Up @@ -583,15 +589,21 @@ protected:

if (_Myfile) { // open C stream, attempt read
_Reset_back(); // revert from _Mychar buffer
// process in 4k - 1 chunks to avoid tripping over fread's clobber-the-end behavior when
// doing \r\n -> \n translation
constexpr size_t _Read_size = 4095; // _INTERNAL_BUFSIZ - 1
while (_Read_size < _Count_s) {
const auto _Actual_read = _CSTD fread(_Ptr, sizeof(_Elem), _Read_size, _Myfile);
_Ptr += _Actual_read;
_Count_s -= _Actual_read;
if (_Actual_read != _Read_size) {
return static_cast<streamsize>(_Start_count - _Count_s);

if (!_Binary) {
// binary mode does not require a \r\n -> \n translation
// fread only 4095 bytes at a time may result in low performance when reading large files

// process in 4k - 1 chunks to avoid tripping over fread's clobber-the-end behavior when
// doing \r\n -> \n translation
constexpr size_t _Read_size = 4095; // _INTERNAL_BUFSIZ - 1
while (_Read_size < _Count_s) {
const auto _Actual_read = _CSTD fread(_Ptr, sizeof(_Elem), _Read_size, _Myfile);
_Ptr += _Actual_read;
_Count_s -= _Actual_read;
if (_Actual_read != _Read_size) {
return static_cast<streamsize>(_Start_count - _Count_s);
}
}
}

Expand Down Expand Up @@ -781,6 +793,7 @@ private:
typename _Traits::state_type _State; // current conversion state
bool _Closef; // true if C stream must be closed
FILE* _Myfile; // pointer to C stream
bool _Binary;// true if open with ios::binary
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this addition breaks ABI compatibility, so can only be done in ABI-breaking vNext release.

Should the compatibility be broken here, could be a good thing to reorder members to put bools together. This likely to reduce the amount of padding bytes for some instantiations. Sure this is very minor, but we are looking even into a minor improvement, if they don't have setbacks.

Copy link
Author

Choose a reason for hiding this comment

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

Thank you for reviewing and your valuable advices.If I fix the ABI compatibility breaking,would it be accepted soon? ^_^

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not a maintainer.
As you can see, there are 78 PRs right now, and there are defect reports, LWG resolutions and bugfixes, which I think have higher priority than perf improvements. On the other hand, some lower-priority changes are occasionally reviewed as well, especially a smaller ones, like #2380
¯\_(ツ)_/¯

I'm also not sure if you can do the same without ABI break.

Copy link
Author

Choose a reason for hiding this comment

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

Sorry... But the std::filebuf‘s low perf is more serious than I described.
For example, when fast reading a 40MB binary file for memory analysis , current version of std::filebuf will call ReadFile 10k times,and need more 40MB copy operation from fread buffer.And there is almost no way to improve, because 4095 is hard-coded.
But using C API fread directly without std::filebuf or std::fstream ,it only calls ReadFile once,and no more copy operation from fread buffer.

Some low level api or code may be useful for doing the same without ABI break .But it may also fail of course. I'm trying.

Copy link
Member

Choose a reason for hiding this comment

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

@AlexGuteniev is correct - adding a data member to basic_filebuf absolutely breaks binary compatibility. (This is because linking old code and new code would have catastrophic consequences at runtime, because functions compiled before and after this change wouldn't agree on the basic_filebuf's state.)

Unfortunately, at this time there is no ETA for the "vNext" ABI-breaking release. 😿 (The maintainers would love to break ABI and fix a whole bunch of bugs, but we need to coordinate with the compiler front-end team, and they were too overloaded with high-priority tasks at the beginning of the VS 2022 release cycle.)

If there's no way to update this PR to improve performance while strictly preserving bincompat, we'll need to close it and instead file a tracking issue labeled vNext to improve things here later.

Copy link
Author

@xth xth Jan 19, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

I did find a way to fix it without breaking binary compatibility,but it's not available in stl for the time being.
The macro "_osfile" in "corecrt_internal_lowio.h" can be used to determine whether a file is in binary mode without adding a data member _Binary.
e.g.

        int fd = fileno(_Myfile);
        bool _Binary = !(_osfile(fd) & FTEXT);

But it needs locating "__pioinfo" which is not exported in ms c runtime.
I believe some reverse engineering methods can still get this variable,but needs time ^_^.
Besides, I really want to delete that piece of code about "4095". There is no evidence that 4095 bytes per read can do \r\n -> \n translation because the underlying fread has done the right job.Is the translation really important for std::filebuf?😿

Copy link
Member

Choose a reason for hiding this comment

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

Thanks - yeah, unfortunately reaching into UCRT internals is something that the STL avoids (although the VC Libraries team co-maintains the UCRT along with the Windows team, we consider the UCRT and STL to be separate libraries that shouldn't access internal data structure representations). We talked about this at the weekly maintainer meeting - I've filed #2490 to track this work for vNext (there's no ETA but it's not happening in the near future, so we'd prefer to track this with an issue instead of keeping a PR open for a year or more). I'll go ahead and close this now.


void _Reset_back() { // restore buffer after putback
if (_Mysb::eback() == &_Mychar) {
Expand Down