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

TokenStream improvements #56737

Merged
merged 6 commits into from
Dec 17, 2018
Merged

Conversation

nnethercote
Copy link
Contributor

Some TokenStream improvements: shrinking TokenStream and some other types, and some other code clean-ups.

@rust-highfive
Copy link
Collaborator

r? @petrochenkov

(rust_highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 12, 2018
@nnethercote
Copy link
Contributor Author

The first commit just replicates #56699, which is a prerequisite. Once that lands I will rebase to remove that commit from this PR.

@rust-highfive
Copy link
Collaborator

The job mingw-check of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
travis_time:end:0499a949:start=1544590199028490396,finish=1544590201310673674,duration=2282183278
$ git checkout -qf FETCH_HEAD
travis_fold:end:git.checkout

Encrypted environment variables have been removed for security reasons.
See https://docs.travis-ci.com/user/pull-requests/#Pull-Requests-and-Security-Restrictions
$ export SCCACHE_BUCKET=rust-lang-ci-sccache2
$ export SCCACHE_REGION=us-west-1
Setting environment variables from .travis.yml
$ export IMAGE=mingw-check
---
[00:03:58]     Checking rustc_data_structures v0.0.0 (/checkout/src/librustc_data_structures)
[00:04:00]     Checking arena v0.0.0 (/checkout/src/libarena)
[00:04:00]     Checking syntax_pos v0.0.0 (/checkout/src/libsyntax_pos)
[00:04:01]     Checking rustc_errors v0.0.0 (/checkout/src/librustc_errors)
[00:04:09] error[E0277]: `std::rc::Rc<std::vec::Vec<tokenstream::TokenStream>>` cannot be sent between threads safely
[00:04:09]    --> src/libsyntax/ext/tt/macro_rules.rs:396:13
[00:04:09] 396 |             expander,
[00:04:09] 396 |             expander,
[00:04:09]     |             ^^^^^^^^ `std::rc::Rc<std::vec::Vec<tokenstream::TokenStream>>` cannot be sent between threads safely
[00:04:09]     |
[00:04:09]     = help: within `(parse::token::Nonterminal, parse::token::LazyTokenStream)`, the trait `std::marker::Send` is not implemented for `std::rc::Rc<std::vec::Vec<tokenstream::TokenStream>>`
[00:04:09]     = note: required because it appears within the type `std::option::Option<std::rc::Rc<std::vec::Vec<tokenstream::TokenStream>>>`
[00:04:09]     = note: required because it appears within the type `tokenstream::ThinTokenStream`
[00:04:09]     = note: required because it appears within the type `tokenstream::TokenTree`
[00:04:09]     = note: required because it appears within the type `parse::token::Nonterminal`
[00:04:09]     = note: required because it appears within the type `(parse::token::Nonterminal, parse::token::LazyTokenStream)`
[00:04:09]     = note: required because of the requirements on the impl of `std::marker::Send` for `std::sync::Arc<(parse::token::Nonterminal, parse::token::LazyTokenStream)>`
[00:04:09]     = note: required because it appears within the type `parse::token::Token`
[00:04:09]     = note: required because it appears within the type `ext::tt::quoted::TokenTree`
[00:04:09]     = note: required because of the requirements on the impl of `std::marker::Send` for `std::ptr::Unique<ext::tt::quoted::TokenTree>`
[00:04:09]     = note: required because it appears within the type `alloc::raw_vec::RawVec<ext::tt::quoted::TokenTree>`
[00:04:09]     = note: required because it appears within the type `std::vec::Vec<ext::tt::quoted::TokenTree>`
[00:04:09]     = note: required because it appears within the type `ext::tt::macro_rules::MacroRulesMacroExpander`
[00:04:09]     = note: required for the cast to the object type `dyn ext::base::TTMacroExpander + std::marker::Send + std::marker::Sync`
[00:04:09] 
[00:04:09] error[E0277]: `std::rc::Rc<std::vec::Vec<tokenstream::TokenStream>>` cannot be shared between threads safely
[00:04:09]    --> src/libsyntax/ext/tt/macro_rules.rs:396:13
[00:04:09] 396 |             expander,
[00:04:09] 396 |             expander,
[00:04:09]     |             ^^^^^^^^ `std::rc::Rc<std::vec::Vec<tokenstream::TokenStream>>` cannot be shared between threads safely
[00:04:09]     |
[00:04:09]     = help: within `(parse::token::Nonterminal, parse::token::LazyTokenStream)`, the trait `std::marker::Sync` is not implemented for `std::rc::Rc<std::vec::Vec<tokenstream::TokenStream>>`
[00:04:09]     = note: required because it appears within the type `std::option::Option<std::rc::Rc<std::vec::Vec<tokenstream::TokenStream>>>`
[00:04:09]     = note: required because it appears within the type `tokenstream::ThinTokenStream`
[00:04:09]     = note: required because it appears within the type `tokenstream::TokenTree`
[00:04:09]     = note: required because it appears within the type `parse::token::Nonterminal`
[00:04:09]     = note: required because it appears within the type `(parse::token::Nonterminal, parse::token::LazyTokenStream)`
[00:04:09]     = note: required because of the requirements on the impl of `std::marker::Send` for `std::sync::Arc<(parse::token::Nonterminal, parse::token::LazyTokenStream)>`
[00:04:09]     = note: required because it appears within the type `parse::token::Token`
[00:04:09]     = note: required because it appears within the type `ext::tt::quoted::TokenTree`
[00:04:09]     = note: required because of the requirements on the impl of `std::marker::Send` for `std::ptr::Unique<ext::tt::quoted::TokenTree>`
[00:04:09]     = note: required because it appears within the type `alloc::raw_vec::RawVec<ext::tt::quoted::TokenTree>`
[00:04:09]     = note: required because it appears within the type `std::vec::Vec<ext::tt::quoted::TokenTree>`
[00:04:09]     = note: required because it appears within the type `ext::tt::macro_rules::MacroRulesMacroExpander`
[00:04:09]     = note: required for the cast to the object type `dyn ext::base::TTMacroExpander + std::marker::Send + std::marker::Sync`
[00:04:09] 
[00:04:09] error[E0277]: `std::rc::Rc<std::vec::Vec<tokenstream::TokenStream>>` cannot be sent between threads safely
[00:04:09]    --> src/libsyntax/ext/tt/macro_rules.rs:408:13
[00:04:09] 408 |             expander,
[00:04:09] 408 |             expander,
[00:04:09]     |             ^^^^^^^^ `std::rc::Rc<std::vec::Vec<tokenstream::TokenStream>>` cannot be sent between threads safely
[00:04:09]     |
[00:04:09]     = help: within `(parse::token::Nonterminal, parse::token::LazyTokenStream)`, the trait `std::marker::Send` is not implemented for `std::rc::Rc<std::vec::Vec<tokenstream::TokenStream>>`
[00:04:09]     = note: required because it appears within the type `std::option::Option<std::rc::Rc<std::vec::Vec<tokenstream::TokenStream>>>`
[00:04:09]     = note: required because it appears within the type `tokenstream::ThinTokenStream`
[00:04:09]     = note: required because it appears within the type `tokenstream::TokenTree`
[00:04:09]     = note: required because it appears within the type `parse::token::Nonterminal`
[00:04:09]     = note: required because it appears within the type `(parse::token::Nonterminal, parse::token::LazyTokenStream)`
[00:04:09]     = note: required because of the requirements on the impl of `std::marker::Send` for `std::sync::Arc<(parse::token::Nonterminal, parse::token::LazyTokenStream)>`
[00:04:09]     = note: required because it appears within the type `parse::token::Token`
[00:04:09]     = note: required because it appears within the type `ext::tt::quoted::TokenTree`
[00:04:09]     = note: required because of the requirements on the impl of `std::marker::Send` for `std::ptr::Unique<ext::tt::quoted::TokenTree>`
[00:04:09]     = note: required because it appears within the type `alloc::raw_vec::RawVec<ext::tt::quoted::TokenTree>`
[00:04:09]     = note: required because it appears within the type `std::vec::Vec<ext::tt::quoted::TokenTree>`
[00:04:09]     = note: required because it appears within the type `ext::tt::macro_rules::MacroRulesMacroExpander`
[00:04:09]     = note: required for the cast to the object type `dyn ext::base::TTMacroExpander + std::marker::Send + std::marker::Sync`
[00:04:09] 
[00:04:09] error[E0277]: `std::rc::Rc<std::vec::Vec<tokenstream::TokenStream>>` cannot be shared between threads safely
[00:04:09]    --> src/libsyntax/ext/tt/macro_rules.rs:408:13
[00:04:09] 408 |             expander,
[00:04:09] 408 |             expander,
[00:04:09]     |             ^^^^^^^^ `std::rc::Rc<std::vec::Vec<tokenstream::TokenStream>>` cannot be shared between threads safely
[00:04:09]     |
[00:04:09]     = help: within `(parse::token::Nonterminal, parse::token::LazyTokenStream)`, the trait `std::marker::Sync` is not implemented for `std::rc::Rc<std::vec::Vec<tokenstream::TokenStream>>`
[00:04:09]     = note: required because it appears within the type `std::option::Option<std::rc::Rc<std::vec::Vec<tokenstream::TokenStream>>>`
[00:04:09]     = note: required because it appears within the type `tokenstream::ThinTokenStream`
[00:04:09]     = note: required because it appears within the type `tokenstream::TokenTree`
[00:04:09]     = note: required because it appears within the type `parse::token::Nonterminal`
[00:04:09]     = note: required because it appears within the type `(parse::token::Nonterminal, parse::token::LazyTokenStream)`
[00:04:09]     = note: required because of the requirements on the impl of `std::marker::Send` for `std::sync::Arc<(parse::token::Nonterminal, parse::token::LazyTokenStream)>`
[00:04:09]     = note: required because it appears within the type `parse::token::Token`
[00:04:09]     = note: required because it appears within the type `ext::tt::quoted::TokenTree`
[00:04:09]     = note: required because of the requirements on the impl of `std::marker::Send` for `std::ptr::Unique<ext::tt::quoted::TokenTree>`
[00:04:09]     = note: required because it appears within the type `alloc::raw_vec::RawVec<ext::tt::quoted::TokenTree>`
[00:04:09]     = note: required because it appears within the type `std::vec::Vec<ext::tt::quoted::TokenTree>`
[00:04:09]     = note: required because it appears within the type `ext::tt::macro_rules::MacroRulesMacroExpander`
[00:04:09]     = note: required for the cast to the object type `dyn ext::base::TTMacroExpander + std::marker::Send + std::marker::Sync`
[00:04:09] error: aborting due to 4 previous errors
[00:04:09] 
[00:04:09] For more information about this error, try `rustc --explain E0277`.
[00:04:09] error: Could not compile `syntax`.
[00:04:09] error: Could not compile `syntax`.
[00:04:09] 
[00:04:09] To learn more, run the command again with --verbose.
[00:04:09] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "check" "--target" "x86_64-unknown-linux-gnu" "-j" "4" "--release" "--color" "always" "--features" "" "--manifest-path" "/checkout/src/rustc/Cargo.toml" "--message-format" "json"
[00:04:09] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap check
[00:04:09] Build completed unsuccessfully in 0:02:41
travis_time:end:10a468be:start=1544590210572672518,finish=1544590460214439220,duration=249641766702
The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 1.
---
travis_time:end:01dcf37c:start=1544590460631927324,finish=1544590460638167393,duration=6240069
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:11e0bde9
$ ln -s . checkout && for CORE in obj/cores/core.*; do EXE=$(echo $CORE | sed 's|obj/cores/core\.[0-9]*\.!checkout!\(.*\)|\1|;y|!|/|'); if [ -f "$EXE" ]; then printf travis_fold":start:crashlog\n\033[31;1m%s\033[0m\n" "$CORE"; gdb --batch -q -c "$CORE" "$EXE" -iex 'set auto-load off' -iex 'dir src/' -iex 'set sysroot .' -ex bt -ex q; echo travis_fold":"end:crashlog; fi; done || true
travis_fold:end:after_failure.4
travis_fold:start:after_failure.5
travis_time:start:05d4745c
travis_time:start:05d4745c
$ cat ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers || true
cat: ./obj/build/x86_64-unknown-linux-gnu/native/asan/build/lib/asan/clang_rt.asan-dynamic-i386.vers: No such file or directory
travis_fold:end:after_failure.5
travis_fold:start:after_failure.6
travis_time:start:0544ccda
$ dmesg | grep -i kill

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@nnethercote
Copy link
Contributor Author

Oh, I need to use Lrc instead of Rc.

This shrinks:
- ThinTokenStream: 16 to 8 bytes
- TokenTree: 32 to 24 bytes
- TokenStream: 40 to 32 bytes

The only downside is that in a couple of places this requires using
`to_vec()` (which allocates) instead of `sub_slice()`. But those places
are rarely executed, so it doesn't hurt perf.

Overall, this reduces instruction counts on numerous benchmarks by up to
3%.
It's a better choice in a few places.
They're both unused now.
Because the distinction provides little value, and removing it cleans up
the code quite a bit.
`TokenStream::new` is a better name for the former, and the latter is
now just equivalent to `TokenStream::Stream`.
@nnethercote
Copy link
Contributor Author

I switched to Lrc.

@petrochenkov
Copy link
Contributor

@bors r+
@bors delegate+

@bors
Copy link
Contributor

bors commented Dec 12, 2018

✌️ @nnethercote can now approve this pull request

@bors
Copy link
Contributor

bors commented Dec 12, 2018

📌 Commit e80c7dd has been approved by petrochenkov

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 12, 2018
@bors
Copy link
Contributor

bors commented Dec 17, 2018

⌛ Testing commit e80c7dd with merge c6fb01d...

bors added a commit that referenced this pull request Dec 17, 2018
…henkov

`TokenStream` improvements

Some `TokenStream` improvements: shrinking `TokenStream` and some other types, and some other code clean-ups.
@bors
Copy link
Contributor

bors commented Dec 17, 2018

☀️ Test successful - status-appveyor, status-travis
Approved by: petrochenkov
Pushing c6fb01d to master...

@bors bors merged commit e80c7dd into rust-lang:master Dec 17, 2018
@nnethercote nnethercote deleted the TokenStream-improvements branch December 17, 2018 20:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants