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

Code Health/Deps: Add Fast_IO library and use it #6350

Closed
WSLUser opened this issue Jun 4, 2020 · 13 comments
Closed

Code Health/Deps: Add Fast_IO library and use it #6350

WSLUser opened this issue Jun 4, 2020 · 13 comments
Labels
Area-Build Issues pertaining to the build system, CI, infrastructure, meta Issue-Task It's a feature request, but it doesn't really need a major design. Product-Meta The product is the management of the products. Resolution-Won't-Fix We're just really obstinate about this. There's probably a good reason.

Comments

@WSLUser
Copy link
Contributor

WSLUser commented Jun 4, 2020

Description of the new feature/enhancement

fast_io is a C++ general-purpose exception-safe RAII I/O library based on C++ 20 Concepts. It is at least ten times faster than cstdio or iostream.

As this is compatible with stdio, cstdio (which has a pending PR still to replace stdio) and iostreams, simply use it for new code and refactorings to boost the performance of I/O in Windows Terminal even further. This will actually allow us to not use fmt for some things (not sure if the fmt calls that are superseded are actually in use yet). As part of getting C++20 compliant (ref: #6251), this library should help get the project there. This is covered under the MIT license so there are no legal issues to worry about.

Proposed technical implementation details (optional)

Go to https://github.com/expnkx/fast_io and simply add the release into dep/ of our project. See the README to get a bunch of performance benchmark comparisons (it's a cross-platform project). There's also a video comparing to some of the fmt stuff. I guess there was some friction between the two project authors: https://www.youtube.com/watch?v=avyIFLuFUQs

@WSLUser WSLUser added the Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. label Jun 4, 2020
@ghost ghost added Needs-Tag-Fix Doesn't match tag requirements Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting labels Jun 4, 2020
@zadjii-msft
Copy link
Member

I'd love to see a perf trace that highlights a place in the codebase where replacing whatever we're currently doing with fast_io would remove some sort of bottleneck.

Would we need to move the entire solution over to C++20 to be able to use this? I was under the impression that C++20 support wasn't quite finished yet. I know that we'll want to use that when we can, but I'd want to make sure the compiler is stable enough first.

@zadjii-msft zadjii-msft added Area-Build Issues pertaining to the build system, CI, infrastructure, meta Help Wanted We encourage anyone to jump in on these. Issue-Task It's a feature request, but it doesn't really need a major design. Product-Meta The product is the management of the products. and removed Issue-Feature Complex enough to require an in depth planning process and actual budgeted, scheduled work. labels Jun 5, 2020
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Jun 5, 2020
@zadjii-msft zadjii-msft added this to the Terminal Backlog milestone Jun 5, 2020
@WSLUser
Copy link
Contributor Author

WSLUser commented Jun 5, 2020

The compiler should be fine as long we're running the latest supported. It illustrates the version tested with one of the benchmark tables for MSVC. I highly suggest looking at the README. The benchmarks include comparing specific std/fmt calls.

Platform Windows MSVC 19.26.28805
Method Output time Input time Comment
stdio.h(fprintf/fscanf) 1.5353597s 1.4157233s  
fstream 3.6350262s 3.8420339s  
fstream with rdbuf.sputc trick 3.3735902s 3.8145566s  
fast_io::i/obuf_file 0.0631433s 0.1030554s  
fast_io::i/obuf_file_mutex 0.2190659s 0.2485886s thread safe
std::to_chars+obuf_file 0.1641641s Meaningless  
std::to_chars+ofstream 0.5461922s Meaningless  
fast_io::c_file_unlocked 0.1102575s 0.2399757s I hacked Universal CRT's FILE* implementation
fast_io::c_file 0.2034755s 0.2621148s Thread Safe. I hacked UCRT's FILE* implementation
fast_io::filebuf_file 0.126661s 0.2378803s I hacked MSVC STL's streambuf/filebuf implementation

@WSLUser
Copy link
Contributor Author

WSLUser commented Jun 5, 2020

It has more things it can fix up with gcc than MSVC, something I think we should go to eventually.

@zadjii-msft
Copy link
Member

Well sure, but are there any hot paths in our codebase currently that are using std::to_chars, fprintf, fstream?

@WSLUser
Copy link
Contributor Author

WSLUser commented Jun 5, 2020

I was hoping @skyline75489 could take a look since he's the perf guy who is more likely to find a place where this could be used or if this is completely not applicable to us at all but I will do some preliminary checking.

@skyline75489
Copy link
Collaborator

Based on my previous experience, most CPU time was used on rendering side. I’m not quite the conpty guy. But I’m getting there. Once I get to know more about conpty, maybe I’ll find a place for it.

@WSLUser
Copy link
Contributor Author

WSLUser commented Jun 5, 2020

It would tremendously help if there was a way to quickly search the codebase for these call strings. If anyone knows a way to do so, I'd appreciate the tip.

@WSLUser
Copy link
Contributor Author

WSLUser commented Jun 5, 2020

So far it appears we aren't doing everything we can for optimizing our C++ code. See https://www.thegeekstuff.com/2015/01/c-cpp-code-optimization/. There have been tweaks and tuning but some things like #5152 aren't in place yet, which would help to some limited degree. Realistically, every optimization we do won't add much on it's own, it's when combined with other optimizations, that it starts to become noticeable. It's probably the reason why issues like #5963 and #3075 exist. So instead of relying on older optimization techniques that would need refactoring for this header-only library, we should just use this library to start with. Fortunately this means my issue is valid exactly as is. It will just be part of a larger issue that needs to be created for optimizing our C++ code.

@zadjii-msft
Copy link
Member

zadjii-msft commented Jun 5, 2020

I know in VSCode and Sublime, you can typically just open up the entire repo directory, and search in all the files:

image
For me, this is bound to ctrl+shift+f


Also, it's almost certainly not the reason #5963 exists. We're pretty confident that's because fundamentally the console isn't just a pair of pipes we can read/write to/from. There's the whole console driver that shuttles API calls between the client and the console host, and that's probably slower than just writing a pipe.

@WSLUser
Copy link
Contributor Author

WSLUser commented Jun 5, 2020

Indeed, that would be another one of those tweaks that helps. I wonder though if when writing a pipe, you could take advantage of fast_io to boost performance of the pipe writing.

@zadjii-msft zadjii-msft removed the Needs-Triage It's a new issue that the core contributor team needs to triage at the next triage meeting label Jun 9, 2020
@vitaut
Copy link

vitaut commented Jun 29, 2020

I guess there was some friction between the two project authors

The {fmt} author here. To clarify the friction part: I called out obviously false performance claims made by the fast_io author (let's say they are using somewhat biased benchmarking methodology =)) and severe code bloat issues with fast_io which unfortunately were ignored. A note of warning: the person is known for their erratic behavior (see e.g. expnkx/fast_io#8) and spamming open-source repositories (isocpp/CppCoreGuidelines#1584) including my own with non-sensical claims.

@vitaut
Copy link

vitaut commented Jun 29, 2020

BTW the main perf advantage of fast_io is avoiding synchronization when writing to a stream which can be done in simpler ways without pulling in the whole library.

@miniksa
Copy link
Member

miniksa commented Jul 14, 2020

I've been pouring over perf traces on things like the PTY for weeks now and nothing in particular stands out as something that needs a specific fast-io library to achieve. I'm also concerned on the testimonials provided by @vitaut. That's not generally the behavior of a library author that meshes with our Microsoft Code of Conduct.

As such, I'm going to close this at this time. Thanks for the idea @WSLUser. If you see specific hot perf routes, please let me know with a trace.

@miniksa miniksa closed this as completed Jul 14, 2020
@ghost ghost added the Needs-Tag-Fix Doesn't match tag requirements label Jul 14, 2020
@miniksa miniksa added the Resolution-Won't-Fix We're just really obstinate about this. There's probably a good reason. label Jul 14, 2020
@ghost ghost removed the Needs-Tag-Fix Doesn't match tag requirements label Jul 14, 2020
@miniksa miniksa removed the Help Wanted We encourage anyone to jump in on these. label Jul 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Build Issues pertaining to the build system, CI, infrastructure, meta Issue-Task It's a feature request, but it doesn't really need a major design. Product-Meta The product is the management of the products. Resolution-Won't-Fix We're just really obstinate about this. There's probably a good reason.
Projects
None yet
Development

No branches or pull requests

5 participants