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

Special-case literals in parse_bottom_expr. #61612

Merged
merged 1 commit into from
Jun 12, 2019
Merged

Special-case literals in parse_bottom_expr. #61612

merged 1 commit into from
Jun 12, 2019

Conversation

nnethercote
Copy link
Contributor

This makes parsing faster, particularly for code with large constants,
for two reasons:

  • it skips all the keyword comparisons for literals;
  • it skips the allocation done by the mk_expr call in
    parse_literal_maybe_minus.

r? @petrochenkov

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

@bors try

@bors
Copy link
Contributor

bors commented Jun 7, 2019

⌛ Trying commit a4952ba with merge ec9590e...

bors added a commit that referenced this pull request Jun 7, 2019
@bors
Auto merge of #61612 - nnethercote:improve-parse_bottom_expr, r=<try>
ec9590e 
Special-case literals in `parse_bottom_expr`.

This makes parsing faster, particularly for code with large constants,
for two reasons:
- it skips all the keyword comparisons for literals;
- it skips the allocation done by the `mk_expr` call in
  `parse_literal_maybe_minus`.

r? @petrochenkov
@bors
Copy link
Contributor

bors commented Jun 7, 2019

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

@bors
Copy link
Contributor

bors commented Jun 7, 2019

☀️ Try build successful - checks-travis
Build commit: ec9590e

@petrochenkov petrochenkov added S-waiting-on-perf Status: Waiting on a perf run to be completed. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jun 7, 2019
@nnethercote
Copy link
Contributor Author

Rebased.

@bors try

@bors
Copy link
Contributor

bors commented Jun 8, 2019

⌛ Trying commit c1947b171c7cedb269b5022f16cd3c92592aceb5 with merge ad061396c7509b3356c9bb5cfa0a8b3f54d9924e...

@nnethercote
Copy link
Contributor Author

@bors rollup=never, because this affects performance.

@bors
Copy link
Contributor

bors commented Jun 8, 2019

☀️ Try build successful - checks-travis
Build commit: ad061396c7509b3356c9bb5cfa0a8b3f54d9924e

@nnethercote
Copy link
Contributor Author

@rust-timer build ad061396c7509b3356c9bb5cfa0a8b3f54d9924e

@rust-timer
Copy link
Collaborator

Success: Queued ad061396c7509b3356c9bb5cfa0a8b3f54d9924e with parent d132f54, comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit ad061396c7509b3356c9bb5cfa0a8b3f54d9924e, comparison URL.

@petrochenkov
Copy link
Contributor

Nice, I didn't expect an effect like this from parser changes.
All the >1% improved tests are small or synthetic though.

Could you move the common code from the added snippet and from its original code below into a closure/function/macro?
parse_literal_maybe_minus below is unnecessary/wrong since we are parsing a bottom expression, so parse_lit needs to be used in both cases.

@petrochenkov petrochenkov 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-perf Status: Waiting on a perf run to be completed. labels Jun 8, 2019
@nnethercote
Copy link
Contributor Author

You mean the parse_literal_maybe_minus in the _ match arm at the bottom of parse_bottom_expr can be parse_lit?

@petrochenkov
Copy link
Contributor

@nnethercote
Yes.

@nnethercote
Special-case literals in parse_bottom_expr.
35b5f43 
This makes parsing faster, particularly for code with large constants,
for two reasons:
- it skips all the keyword comparisons for literals;
- it replaces the unnecessary `parse_literal_maybe_minus` call with
  `parse_lit`, avoiding an unnecessary allocation via `mk_expr`.
@petrochenkov
Copy link
Contributor

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Jun 10, 2019

📌 Commit 35b5f43 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jun 10, 2019
@nnethercote
Copy link
Contributor Author

@bors rollup=never, because this affects performance.

@nnethercote
Copy link
Contributor Author

Trying again...

@bors rollup=never

@nnethercote
Copy link
Contributor Author

Ok, that worked. Looks like the reason for the rollup-never must not be on the same line.

Centril added a commit to Centril/rust that referenced this pull request Jun 10, 2019
@Centril
Rollup merge of rust-lang#61612 - nnethercote:improve-parse_bottom_ex…
965e75a 
…pr, r=petrochenkov

Special-case literals in `parse_bottom_expr`.

This makes parsing faster, particularly for code with large constants,
for two reasons:
- it skips all the keyword comparisons for literals;
- it skips the allocation done by the `mk_expr` call in
  `parse_literal_maybe_minus`.

r? @petrochenkov
@Centril Centril mentioned this pull request Jun 10, 2019
Closed
bors added a commit that referenced this pull request Jun 12, 2019
@bors
Auto merge of #61612 - nnethercote:improve-parse_bottom_expr, r=petro…
55cee44 
…chenkov

Special-case literals in `parse_bottom_expr`.

This makes parsing faster, particularly for code with large constants,
for two reasons:
- it skips all the keyword comparisons for literals;
- it skips the allocation done by the `mk_expr` call in
  `parse_literal_maybe_minus`.

r? @petrochenkov
@bors
Copy link
Contributor

bors commented Jun 12, 2019

⌛ Testing commit 35b5f43 with merge 55cee44...

@bors
Copy link
Contributor

bors commented Jun 12, 2019

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

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jun 12, 2019
@bors bors merged commit 35b5f43 into rust-lang:master Jun 12, 2019
@rust-highfive
Copy link
Collaborator

📣 Toolstate changed by #61612!

Tested on commit 55cee44.
Direct link to PR: #61612

🎉 rls on linux: test-fail → test-pass (cc @Xanewok, @rust-lang/infra).

rust-highfive added a commit to rust-lang-nursery/rust-toolstate that referenced this pull request Jun 12, 2019
@rust-highfive
📣 Toolstate changed by rust-lang/rust#61612!
aecd32d 
Tested on commit rust-lang/rust@55cee44.
Direct link to PR: <rust-lang/rust#61612>

🎉 rls on linux: test-fail → test-pass (cc @Xanewok, @rust-lang/infra).
@nnethercote nnethercote deleted the improve-parse_bottom_expr branch June 12, 2019 22:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@petrochenkov petrochenkov

Labels
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.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@nnethercote @bors @rust-timer @petrochenkov @rust-highfive