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

Conversation

Projects
None yet
8 participants
@nnethercote
Copy link
Contributor

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

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Dec 20, 2018

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

This comment has been minimized.

Copy link
Contributor

bors commented Dec 24, 2018

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

@nnethercote nnethercote force-pushed the nnethercote:TS-change-Stream branch from e2e57bb to facc038 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.

@nnethercote nnethercote force-pushed the nnethercote:TS-change-Stream branch from facc038 to e80a930 Jan 8, 2019

@nnethercote nnethercote changed the title De-recursify `TokenStream` Make `TokenStream` less recursive. Jan 8, 2019

@nnethercote

This comment has been minimized.

Copy link
Contributor

nnethercote commented Jan 8, 2019

New code is ready for review.

r? @eddyb

@petrochenkov

This comment has been minimized.

Copy link
Contributor

petrochenkov commented Jan 8, 2019

(eddyb is absent, reassigning to me.)

@petrochenkov petrochenkov assigned petrochenkov and unassigned eddyb Jan 8, 2019

@petrochenkov

This comment has been minimized.

Copy link
Contributor

petrochenkov commented Jan 8, 2019

A bit of a history:

  • Originally token streams used simple Vec<TokenTree> (#34575).
  • Then they were reimplemented as ropes as an optimization (#35018), apparently to increase sharing, but this also introduced recursive token streams and complexity. It looks like no benchmarking was done for that change.
  • Then ropes were replaced with RcSlice<TokenStream> (#39173), simplifying the code, but keeping the recursive token streams. Looks like no benchmarking was done for that change either.
  • Then RcSlice turned into RcVec (#53304), and then into Lrc<Vec> (#56737).
  • Now this PR completes the cycle and returns to the original implementation. So, we shouldn't repeat the past errors and at least benchmark it 😄
@petrochenkov

This comment has been minimized.

Copy link
Contributor

petrochenkov commented Jan 8, 2019

@bors try

@bors

This comment has been minimized.

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

Auto merge of #57004 - nnethercote:TS-change-Stream, r=<try>
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

This comment has been minimized.

Copy link
Contributor

bors commented Jan 8, 2019

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

@petrochenkov

This comment has been minimized.

Copy link
Contributor

petrochenkov commented Jan 8, 2019

@rust-timer

This comment has been minimized.

Copy link

rust-timer commented Jan 8, 2019

Insufficient permissions to issue commands to rust-timer.

@pietroalbini

This comment has been minimized.

Copy link
Member

pietroalbini commented Jan 8, 2019

@rust-timer

This comment has been minimized.

Copy link

rust-timer commented Jan 8, 2019

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

@rust-timer

This comment has been minimized.

Copy link

rust-timer commented Jan 8, 2019

Finished benchmarking try commit 7346647

@nnethercote

This comment has been minimized.

Copy link
Contributor

nnethercote commented Jan 8, 2019

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

This comment has been minimized.

Copy link
Contributor

petrochenkov commented Jan 8, 2019

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

This comment has been minimized.

Copy link
Contributor

bors commented Jan 8, 2019

📌 Commit e80a930 has been approved by petrochenkov

@bors

This comment has been minimized.

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

Auto merge of #57004 - nnethercote:TS-change-Stream, r=petrochenkov
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

This comment has been minimized.

Copy link
Contributor

bors commented Jan 9, 2019

💥 Test timed out

@Mark-Simulacrum

This comment has been minimized.

Copy link
Member

Mark-Simulacrum commented Jan 10, 2019

@bors retry

Feel free to issue a retry (you should have permission; same requirement as try builds iirc) when infra fails/spurious issues occur.

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 10, 2019

⌛️ Testing commit e80a930 with merge 8090a9c...

bors added a commit that referenced this pull request Jan 10, 2019

Auto merge of #57004 - nnethercote:TS-change-Stream, r=petrochenkov
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

This comment has been minimized.

Copy link
Contributor

bors commented Jan 10, 2019

💔 Test failed - status-appveyor

@nnethercote

This comment has been minimized.

Copy link
Contributor

nnethercote commented Jan 10, 2019

broken_heart Test failed - status-appveyor

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

@bors retry

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 10, 2019

⌛️ Testing commit e80a930 with merge 7062e23...

bors added a commit that referenced this pull request Jan 10, 2019

Auto merge of #57004 - nnethercote:TS-change-Stream, r=petrochenkov
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

This comment has been minimized.

Copy link
Contributor

bors commented Jan 10, 2019

💔 Test failed - status-appveyor

@pietroalbini

This comment has been minimized.

Copy link
Member

pietroalbini commented Jan 10, 2019

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

Centril added a commit to Centril/rust that referenced this pull request Jan 13, 2019

Rollup merge of rust-lang#57004 - nnethercote:TS-change-Stream, r=pet…
…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.

Centril added a commit to Centril/rust that referenced this pull request Jan 13, 2019

Rollup merge of rust-lang#57004 - nnethercote:TS-change-Stream, r=pet…
…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.

Centril added a commit to Centril/rust that referenced this pull request Jan 13, 2019

Rollup merge of rust-lang#57004 - nnethercote:TS-change-Stream, r=pet…
…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.

Centril added a commit to Centril/rust that referenced this pull request Jan 13, 2019

Rollup merge of rust-lang#57004 - nnethercote:TS-change-Stream, r=pet…
…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

Auto merge of #57577 - Centril:rollup, r=Centril
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

1 of 2 checks passed

homu Test failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@nnethercote nnethercote deleted the nnethercote:TS-change-Stream branch Jan 13, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment