Performance Tuning of std::filebuf #2481
Conversation
Binary mode does not require a \r\n -> \n translation Reading only 4095 bytes at a time may result in low performance when reading large files Signed-off-by: xth <[email protected]>
|
|
|
run |
There was a problem hiding this comment.
Thank you for performance improvement!
Unfortunately I don't think it will be accepted soon because it is an ABI change, so it is blocked on #169
| @@ -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 | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Thank you for reviewing and your valuable advices.If I fix the ABI compatibility breaking,would it be accepted soon? ^_^
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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?😿
There was a problem hiding this comment.
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.
| @@ -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; | |||
There was a problem hiding this comment.
We use this-> only when it is required to find template base member. Occurs below.
| auto _Binary_sav = _Binary; | ||
| _Binary = _Right._Binary; | ||
| _Right._Binary = _Binary_sav; |
There was a problem hiding this comment.
Please use clang-format
Binary mode of std::filebuf does not require a \r\n -> \n translation
Reading only 4095 bytes at a time may result in low performance when reading large files
Signed-off-by: xth [email protected]