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

Make TokenStream less recursive. #57004

Merged
merged 1 commit into from
Jan 13, 2019
Merged

Conversation

nnethercote
Copy link
Contributor

@nnethercote nnethercote commented Dec 20, 2018

TokenStream is currently recursive in two ways:

  • the TokenTree variant contains a ThinTokenStream, which can
    contain a TokenStream;

  • the TokenStream variant contains a Vec<TokenStream>.

The latter is not necessary and causes significant complexity. This
commit replaces it with the simpler Vec<(TokenTree, IsJoint)>.

This reduces complexity significantly. In particular, StreamCursor is
eliminated, and Cursor becomes much simpler, consisting now of just a
TokenStream and an index.

The commit also removes the Extend impl for TokenStream, because it
is only used in tests. (The commit also removes those tests.)

Overall, the commit reduces the number of lines of code by almost 200.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 20, 2018
@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:054da1d2:start=1545297245717990325,finish=1545297303072910640,duration=57354920315
$ 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:05:51] configure: build.locked-deps    := True
[00:05:51] configure: llvm.ccache          := sccache
[00:05:51] configure: build.cargo-native-static := True
[00:05:51] configure: dist.missing-tools   := True
[00:05:51] configure: build.configure-args := ['--enable-sccache', '--disable-manage-submodu ...
[00:05:51] configure: writing `config.toml` in current directory
[00:05:51] configure: 
[00:05:51] configure: run `python /checkout/x.py --help`
[00:05:51] configure: 
---
[00:08:15]     Checking rustc_errors v0.0.0 (/checkout/src/librustc_errors)
[00:08:18] error: unused import: `mem`
[00:08:18]   --> src/libsyntax/tokenstream.rs:35:22
[00:08:18]    |
[00:08:18] 35 | use std::{fmt, iter, mem};
[00:08:18]    |
[00:08:18]    = note: `-D unused-imports` implied by `-D warnings`
[00:08:18] 
[00:08:23] error: aborting due to previous error
[00:08:23] error: aborting due to previous error
[00:08:23] 
[00:08:23] error: Could not compile `syntax`.
[00:08:23] 
[00:08:23] To learn more, run the command again with --verbose.
[00:08:23] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "check" "--target" "i686-pc-windows-gnu" "-j" "4" "--release" "--locked" "--color" "always" "--features" "" "--manifest-path" "/checkout/src/rustc/Cargo.toml" "--message-format" "json"
[00:08:23] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap check --target=i686-pc-windows-gnu --host=i686-pc-windows-gnu
[00:08:23] Build completed unsuccessfully in 0:01:21
travis_time:end:0015db30:start=1545297311720268660,finish=1545297815425529278,duration=503705260618
The command "stamp sh -x -c "$RUN_SCRIPT"" exited with 1.
---
travis_time:end:0bb6472a:start=1545297815908039571,finish=1545297815914791154,duration=6751583
travis_fold:end:after_failure.3
travis_fold:start:after_failure.4
travis_time:start:111da833
$ 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:1a7ae108
travis_time:start:1a7ae108
$ 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:062b8bd3
$ 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)

@bors
Copy link
Contributor

bors commented Dec 24, 2018

☔ The latest upstream changes (presumably #57087) made this pull request unmergeable. Please resolve the merge conflicts.

`TokenStream` is currently recursive in *two* ways:

- the `TokenTree` variant contains a `ThinTokenStream`, which can
  contain a `TokenStream`;

- the `TokenStream` variant contains a `Vec<TokenStream>`.

The latter is not necessary and causes significant complexity. This
commit replaces it with the simpler `Vec<(TokenTree, IsJoint)>`.

This reduces complexity significantly. In particular, `StreamCursor` is
eliminated, and `Cursor` becomes much simpler, consisting now of just a
`TokenStream` and an index.

The commit also removes the `Extend` impl for `TokenStream`, because it
is only used in tests. (The commit also removes those tests.)

Overall, the commit reduces the number of lines of code by almost 200.
@nnethercote nnethercote changed the title De-recursify TokenStream Make TokenStream less recursive. Jan 8, 2019
@nnethercote
Copy link
Contributor Author

New code is ready for review.

r? @eddyb

@petrochenkov
Copy link
Contributor

(eddyb is absent, reassigning to me.)

@petrochenkov petrochenkov assigned petrochenkov and unassigned eddyb Jan 8, 2019
@petrochenkov
Copy link
Contributor

A bit of a history:

@petrochenkov
Copy link
Contributor

@bors try

@bors
Copy link
Contributor

bors commented Jan 8, 2019

⌛ Trying commit e80a930 with merge 7346647...

bors added a commit that referenced this pull request Jan 8, 2019
Make `TokenStream` less recursive.

`TokenStream` is currently recursive in *two* ways:

- the `TokenTree` variant contains a `ThinTokenStream`, which can
  contain a `TokenStream`;

- the `TokenStream` variant contains a `Vec<TokenStream>`.

The latter is not necessary and causes significant complexity. This
commit replaces it with the simpler `Vec<(TokenTree, IsJoint)>`.

This reduces complexity significantly. In particular, `StreamCursor` is
eliminated, and `Cursor` becomes much simpler, consisting now of just a
`TokenStream` and an index.

The commit also removes the `Extend` impl for `TokenStream`, because it
is only used in tests. (The commit also removes those tests.)

Overall, the commit reduces the number of lines of code by almost 200.
@bors
Copy link
Contributor

bors commented Jan 8, 2019

☀️ Test successful - status-travis
State: approved= try=True

@petrochenkov
Copy link
Contributor

@rust-timer build 7346647

@rust-timer
Copy link
Collaborator

Insufficient permissions to issue commands to rust-timer.

@pietroalbini
Copy link
Member

@rust-timer build 7346647

@rust-timer
Copy link
Collaborator

Success: Queued 7346647 with parent 7ad470c, comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit 7346647

@nnethercote
Copy link
Contributor Author

The performance results are basically a wash: some minor improvements, some minor regressions. (Which matches what I saw locally.)

But note that the regressions are almost entirely within the short-running (and less interesting) benchmarks: issue-46449, regression-31157, unify-linearly, deeply-nested, unused-warnings, helloworld. The "real world" benchmarks were largely unaffected or slightly improved.

@petrochenkov
Copy link
Contributor

I'd be entirely happy to merge this simplification if performance is unaffected.

Since @cgswords is no longer active, pinging @nrc who approved #35018, please r- if you have concerns.

@bors r+

@bors
Copy link
Contributor

bors commented Jan 8, 2019

📌 Commit e80a930 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 Jan 8, 2019
@bors
Copy link
Contributor

bors commented Jan 9, 2019

⌛ Testing commit e80a930 with merge bd981cc...

bors added a commit that referenced this pull request Jan 9, 2019
Make `TokenStream` less recursive.

`TokenStream` is currently recursive in *two* ways:

- the `TokenTree` variant contains a `ThinTokenStream`, which can
  contain a `TokenStream`;

- the `TokenStream` variant contains a `Vec<TokenStream>`.

The latter is not necessary and causes significant complexity. This
commit replaces it with the simpler `Vec<(TokenTree, IsJoint)>`.

This reduces complexity significantly. In particular, `StreamCursor` is
eliminated, and `Cursor` becomes much simpler, consisting now of just a
`TokenStream` and an index.

The commit also removes the `Extend` impl for `TokenStream`, because it
is only used in tests. (The commit also removes those tests.)

Overall, the commit reduces the number of lines of code by almost 200.
@bors
Copy link
Contributor

bors commented Jan 9, 2019

💥 Test timed out

@bors
Copy link
Contributor

bors commented Jan 10, 2019

⌛ Testing commit e80a930 with merge 8090a9c09bf365105ef844d61a2d69124e2c56fa...

@bors
Copy link
Contributor

bors commented Jan 10, 2019

💔 Test failed - status-appveyor

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

broken_heart Test failed - status-appveyor

No idea about that, but it seems spurious, so...

@bors retry

@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 Jan 10, 2019
@bors
Copy link
Contributor

bors commented Jan 10, 2019

⌛ Testing commit e80a930 with merge 7062e23180480c2f15a1f27a6d444b76b0419bd9...

@bors
Copy link
Contributor

bors commented Jan 10, 2019

💔 Test failed - status-appveyor

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Jan 10, 2019
@pietroalbini
Copy link
Member

@bors retry
AppVeyor... what's wrong with you today?

@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 Jan 10, 2019
Centril added a commit to Centril/rust that referenced this pull request Jan 13, 2019
…rochenkov

Make `TokenStream` less recursive.

`TokenStream` is currently recursive in *two* ways:

- the `TokenTree` variant contains a `ThinTokenStream`, which can
  contain a `TokenStream`;

- the `TokenStream` variant contains a `Vec<TokenStream>`.

The latter is not necessary and causes significant complexity. This
commit replaces it with the simpler `Vec<(TokenTree, IsJoint)>`.

This reduces complexity significantly. In particular, `StreamCursor` is
eliminated, and `Cursor` becomes much simpler, consisting now of just a
`TokenStream` and an index.

The commit also removes the `Extend` impl for `TokenStream`, because it
is only used in tests. (The commit also removes those tests.)

Overall, the commit reduces the number of lines of code by almost 200.
bors added a commit that referenced this pull request Jan 13, 2019
Rollup of 4 pull requests

Successful merges:

 - #57004 (Make `TokenStream` less recursive.)
 - #57102 (NLL: Add union justifications to conflicting borrows.)
 - #57337 (rustc: Place wasm linker args first instead of last)
 - #57549 (Add #[must_use] message to Iterator and Future)

Failed merges:

r? @ghost
@bors bors merged commit e80a930 into rust-lang:master Jan 13, 2019
@nnethercote nnethercote deleted the TS-change-Stream branch January 13, 2019 22:28
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.

8 participants