<iterator>: reduced parsing time #355
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
StephanTLavavej
approved these changes
Dec 6, 2019
There was a problem hiding this comment.
Thanks for the compiler throughput measurements! I find that significant enough, and the scenario to be reasonable enough, for this to be worth the source breaking change. (I also hope that not too many people are depending on <iterator> dragging in <istream>.) I think we should merge this change, although we may need to revert it if it breaks too many "Real World Code" projects in our overall test suite.
|
For the reference, libstdc++ did the same thing gcc-mirror/gcc@e0d6537 (though they still pull bunch of unneeded stuff) |
CaseyCarter
approved these changes
Dec 6, 2019
|
I don't think this change actually works: since |
BillyONeal
suggested changes
Dec 6, 2019
|
|
|
Yeah, but the streams actually use streambuf_iterator, that's why it's defined there today. I think we would need to introduce a new header that has just *streambuf_iterator, and define the member functions of it in near where they already are (since they need streambuf to be complete). |
|
N4842 [iterator.synopsis] requires Line 11 in da0d8cf Line 11 in da0d8cf Line 11 in da0d8cf Line 11 in da0d8cf Line 15 in da0d8cf Lines 414 to 415 in da0d8cf Lines 537 to 538 in da0d8cf I wonder whether introducing a new internal header is necessary. Could the definitions simply be moved, with appropriate forward declarations (although the two-phase lookup rules might not make this easy/possible)? Splitting up a class definition and its member function definitions is unusual (we virtually never do that). |
StephanTLavavej
requested changes
Dec 6, 2019
|
I can try to sort this out. Is including |
Go for it - I agree that the additional cost should be very small, and those headers are already heavyweight. |
|
Yeah, this header is small enough that there's no need to create something else if making it work can be done without introduction of circular dependencies. It might even make sense to delete |
|
Ah, I take back my |
|
And |
|
|
Kojoley
changed the title
|
Updated the PR. Stats are in the first post. Also verified with libc++ |
* Replaced `<istream>` include with `<iosfwd>` because `[io]stream_iterator` needs only `basic_[io]stream` forward declaration. * Moved `[io]streambuf_iterator` iterator definition to `<iterator>` because their definition has to come when `<iterator>` is included. * Include `<iterator>` in `<xlocmon>`, `<xlocnum>`, and `<xloctime>` as the `[io]streambuf_iterator` definition are required there, and `<xutility>` already included via other includes. Parsing times: | header | clang | msvc | |------------|-------------------------|-------------------------| | <iterator> | 0.371 -> 0.163 (-56.1%) | 0.216 -> 0.094 (-56.5%) | | <istream> | 0.366 -> 0.372 ( +1.6%) | 0.215 -> 0.216 ( +0.5%) | | <xlocmon> | 0.358 -> 0.364 ( +1.7%) | 0.211 -> 0.211 ( 0%) | | <xlocnum> | 0.357 -> 0.360 ( +0.8%) | 0.207 -> 0.208 ( +0.5%) | | <xloctime> | 0.364 -> 0.370 ( +1.6%) | 0.211 -> 0.214 ( +1.4%) |
StephanTLavavej
approved these changes
Dec 10, 2019
| @@ -8,6 +8,7 @@ | |||
| #define _XLOCMON_ | |||
| #include <yvals_core.h> | |||
| #if _STL_COMPILER_PREPROCESSOR | |||
| #include <iterator> | |||
There was a problem hiding this comment.
Including <iterator> is unnecessary in <xlocmon> and <xloctime>, and it's slightly inconsistent with the existing code that appears to treat <xlocnum> as central. However, I'm fine with this change. (STL headers are wildly inconsistent about assuming transitive inclusion, although we try to be very clean in our tests.)
|
There's at least one file within the compiler's sources that was using I'm fixing them up, but I believe that we should merge this change right after VS 2019 16.5 Preview 2 branches for release (so we have all of the 16.6 cycle to deal with the fallout in Real World Code). |
Derp, sorry for sending you on that goose chase. I had already prepared the same fix :P https://devdiv.visualstudio.com/DefaultCollection/DevDiv/_git/msvc/commit/c72ac3991d63213948afe51926c1c6bb2dd74003?refName=refs%2Fheads%2Fdev%2Fbion%2Fgh-355 |
BillyONeal
approved these changes
Dec 10, 2019
There was a problem hiding this comment.
As long as @StephanTLavavej indicates that this passed testing, LGTM
|
Two more failure categories in libcxx's test suite:
|
|
I've gotten this to pass our internal testing. I'm planning to wait until after Friday (2019-12-13) to merge this, so that the source breaking changes will appear in VS 2019 16.6. (This is partially because making source breaking changes at the beginning of a release cycle is better than at the end, and partially because 16.5 will be the first release with STL GitHub changes, and it would be nice for it to be minimally disruptive.) |
CaseyCarter
approved these changes
Dec 11, 2019
|
Fixed libcxx tests upstream: llvm/llvm-project@5688f16 Created Microsoft-internal MSVC-PR-218964. |
|
I've merged this for VS 2019 16.6. We'll see in the next few days how many open-source projects in our "Real World Code" test suite this breaks; we'll submit PRs to those projects if the number of breaks is fairly low, but we may need to consider reverting this throughput improvement if the source breaking impact is enormous. Thanks for the improvement, and congratulations on your first microsoft/STL commit! |
|
Thanks for merging! |
fengjixuchui
added a commit
to fengjixuchui/STL
that referenced
this pull request
Dec 17, 2019
<iterator>: reduced parsing time (microsoft#355)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
<istream>include with<iosfwd>because[io]stream_iteratorneeds only
basic_[io]streamforward declaration.[io]streambuf_iteratoriterator definition to<iterator>becausetheir definition has to come when
<iterator>is included.<iterator>in<xlocmon>,<xlocnum>, and<xloctime>asthe
[io]streambuf_iteratordefinition are required there, and<xutility>already included via other includes.
Parsing times (>1ms difference filter):
<iterator><array><regex><functional><unordered_set><future><ostream><locale><fstream><strstream><iostream><sstream><codecvt><istream><iomanip><complex>Checklist
Be sure you've read README.md and understand the scope of this repo.
If you're unsure about a box, leave it unchecked. A maintainer will help you.
_Uglyas perhttps://eel.is/c++draft/lex.name#3.1 or there are no product code changes.
verified by an STL maintainer before automated testing is enabled on GitHub,
leave this unchecked for initial submission).
members, adding virtual functions, changing whether a type is an aggregate
or trivially copyable, etc.).
the C++ Working Draft (including any cited standards), other WG21 papers
(excluding reference implementations outside of proposed standard wording),
and LWG issues as reference material. If they were derived from a project
that's already listed in NOTICE.txt, that's fine, but please mention it.
If they were derived from any other project (including Boost and libc++,
which are not yet listed in NOTICE.txt), you must mention it here,
so we can determine whether the license is compatible and what else needs
to be done.