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.

Sign up for GitHub

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

Jump to bottom

rustc_parse: Remove optimization for 0-length streams in collect_tokens #78736

Merged
merged 1 commit into from
Nov 14, 2020
Merged

rustc_parse: Remove optimization for 0-length streams in collect_tokens #78736

merged 1 commit into from
Nov 14, 2020

Conversation

petrochenkov
Copy link
Contributor

@petrochenkov petrochenkov commented Nov 4, 2020 •
edited
Loading

The optimization conflates empty token streams with unknown token stream, which is at least suspicious, and doesn't affect performance because 0-length token streams are very rare.

r? @Aaron1011

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 4, 2020
@petrochenkov
Copy link
Contributor Author

@bors try @rust-timer queue

@rust-timer
Copy link
Collaborator

Awaiting bors try build completion

@bors
Copy link
Contributor

bors commented Nov 4, 2020

⌛ Trying commit 77b9d9d4916afed142696d5d02640c940b472cae with merge c0412539ea5cbf39d8ba7b0f55e19f5dcefc3c62...

@bors
Copy link
Contributor

bors commented Nov 4, 2020

☀️ Try build successful - checks-actions
Build commit: c0412539ea5cbf39d8ba7b0f55e19f5dcefc3c62 (c0412539ea5cbf39d8ba7b0f55e19f5dcefc3c62)

@rust-timer
Copy link
Collaborator

Queued c0412539ea5cbf39d8ba7b0f55e19f5dcefc3c62 with parent 601c13c, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (c0412539ea5cbf39d8ba7b0f55e19f5dcefc3c62): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

@bors rollup=never
@rustbot modify labels: +S-waiting-on-review -S-waiting-on-perf

@Aaron1011 Aaron1011 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 5, 2020
@petrochenkov
Copy link
Contributor Author

petrochenkov commented Nov 5, 2020 •
edited
Loading

Some more statistics - number of calls to collect_tokens and to create_token_stream for different values of num_calls.
Looks like lazy tokens streams are pretty rarely converted to real token streams.
Figure_1

@petrochenkov
Copy link
Contributor Author

This means that

  • we perhaps need to find a way to collect tokens in less situations
  • make collection as cheap as possible in the meantime

Right now the construction of LazyTokenStreamImpl is free, and this PR introduces 2 more allocations and deallocations for all 1-token streams, which are the most common ones.

@petrochenkov petrochenkov mentioned this pull request Nov 5, 2020
Merged
@petrochenkov
Copy link
Contributor Author

Marking as blocked on #78782, I'll re-collect the statistics after that PR lands.

@petrochenkov petrochenkov added S-blocked Status: Blocked on something else such as an RFC or other implementation work. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 5, 2020
@petrochenkov petrochenkov mentioned this pull request Nov 5, 2020
Merged
@jyn514 jyn514 added I-compiletime Issue: Problems and improvements with respect to compile times. A-parser Area: The parsing of Rust source code to an AST T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Nov 6, 2020
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 12, 2020
@bors
Auto merge of rust-lang#78782 - petrochenkov:nodoctok, r=Aaron1011
5a6a41e 
Do not collect tokens for doc comments

Doc comment is a single token and AST has all the information to re-create it precisely.
Doc comments are also responsible for majority of calls to `collect_tokens` (with `num_calls == 1` and `num_calls == 0`, cc rust-lang#78736).

(I also moved token collection into `fn parse_attribute` to deduplicate code a bit.)

r? `@Aaron1011`
flip1995 pushed a commit to flip1995/rust-clippy that referenced this pull request Nov 12, 2020
@bors
Auto merge of #78782 - petrochenkov:nodoctok, r=Aaron1011
92ece84 
Do not collect tokens for doc comments

Doc comment is a single token and AST has all the information to re-create it precisely.
Doc comments are also responsible for majority of calls to `collect_tokens` (with `num_calls == 1` and `num_calls == 0`, cc rust-lang/rust#78736).

(I also moved token collection into `fn parse_attribute` to deduplicate code a bit.)

r? `@Aaron1011`
@petrochenkov petrochenkov removed the S-blocked Status: Blocked on something else such as an RFC or other implementation work. label Nov 12, 2020
@bors
Copy link
Contributor

bors commented Nov 12, 2020

☀️ Try build successful - checks-actions
Build commit: 8b963349bc7968741e1cec1bfdc52e0f9629d35a (8b963349bc7968741e1cec1bfdc52e0f9629d35a)

@rust-timer
Copy link
Collaborator

Queued 8b963349bc7968741e1cec1bfdc52e0f9629d35a with parent 9722952, future comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit (8b963349bc7968741e1cec1bfdc52e0f9629d35a): comparison url.

Benchmarking this pull request likely means that it is perf-sensitive, so we're automatically marking it as not fit for rolling up. Please note that if the perf results are neutral, you should likely undo the rollup=never given below by specifying rollup- to bors.

Importantly, though, if the results of this run are non-neutral do not roll this PR up -- it will mask other regressions or improvements in the roll up.

@bors rollup=never
@rustbot modify labels: +S-waiting-on-review -S-waiting-on-perf

@Aaron1011
Copy link
Member

@bors rollup-

@Aaron1011
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Nov 12, 2020

📌 Commit 2879ab7 has been approved by Aaron1011

@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 Nov 12, 2020
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Nov 13, 2020
@Dylan-DPC
Rollup merge of rust-lang#78736 - petrochenkov:lazyenum, r=Aaron1011
4e93b53 
rustc_parse: Remove optimization for 0-length streams in `collect_tokens`

The optimization conflates empty token streams with unknown token stream, which is at least suspicious, and doesn't affect performance because 0-length token streams are very rare.

r? `@Aaron1011`
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Nov 13, 2020
@Dylan-DPC
Rollup merge of rust-lang#78736 - petrochenkov:lazyenum, r=Aaron1011
c71ea50 
rustc_parse: Remove optimization for 0-length streams in `collect_tokens`

The optimization conflates empty token streams with unknown token stream, which is at least suspicious, and doesn't affect performance because 0-length token streams are very rare.

r? ``@Aaron1011``
This was referenced Nov 13, 2020
Closed
Closed
@bors
Copy link
Contributor

bors commented Nov 13, 2020

⌛ Testing commit 2879ab7 with merge dadec646cd5075aed7357933089ba1d2ab0a1545...

@rust-log-analyzer
Copy link
Collaborator

The job dist-i686-msvc of your PR failed (pretty log, 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. 
[command]"C:\Program Files\Git\bin\git.exe" submodule foreach --recursive "git config --local --name-only --get-regexp 'http\.https\:\/\/github\.com\/\.extraheader' && git config --local --unset-all 'http.https://github.com/.extraheader' || :"
[command]"C:\Program Files\Git\bin\git.exe" config --local http.https://github.com/.extraheader "AUTHORIZATION: basic ***"
##[endgroup]
##[group]Fetching the repository
[command]"C:\Program Files\Git\bin\git.exe" -c protocol.version=2 fetch --no-tags --prune --progress --no-recurse-submodules --depth=2 origin +dadec646cd5075aed7357933089ba1d2ab0a1545:refs/remotes/origin/auto

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 @rust-lang/infra. (Feature Requests)

@bors
Copy link
Contributor

bors commented Nov 13, 2020

💔 Test failed - checks-actions

@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 Nov 13, 2020
@Aaron1011
Copy link
Member

spurious zlib inflation error

@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 Nov 13, 2020
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Nov 14, 2020
@Dylan-DPC
Rollup merge of rust-lang#78736 - petrochenkov:lazyenum, r=Aaron1011
6272915 
rustc_parse: Remove optimization for 0-length streams in `collect_tokens`

The optimization conflates empty token streams with unknown token stream, which is at least suspicious, and doesn't affect performance because 0-length token streams are very rare.

r? `@Aaron1011`
@Dylan-DPC-zz Dylan-DPC-zz mentioned this pull request Nov 14, 2020
Closed
@bors
Copy link
Contributor

bors commented Nov 14, 2020

⌛ Testing commit 2879ab7 with merge 50d3c2a...

@bors
Copy link
Contributor

bors commented Nov 14, 2020

☀️ Test successful - checks-actions
Approved by: Aaron1011
Pushing 50d3c2a to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 14, 2020
@bors bors merged commit 50d3c2a into rust-lang:master Nov 14, 2020
@rustbot rustbot added this to the 1.50.0 milestone Nov 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Aaron1011 Aaron1011 Aaron1011 left review comments

Assignees

@Aaron1011 Aaron1011

Labels
A-parser Area: The parsing of Rust source code to an AST I-compiletime Issue: Problems and improvements with respect to compile times. merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.50.0
Development

Successfully merging this pull request may close these issues.
8 participants
@petrochenkov @rust-timer @bors @Aaron1011 @rust-log-analyzer @rust-highfive @jyn514 @rustbot