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

Speed up expansion #42533

Merged
merged 1 commit into from
Jun 10, 2017
Merged

Conversation

Mark-Simulacrum
Copy link
Member

This reduces duplication, thereby increasing expansion speed. Based on tests with rust-uinput, this produces a 29x performance win (440 seconds to 15 seconds). I want to land this first, since it's a minimal patch, but with more changes to the macro parsing I can get down to 12 seconds locally.

There is one FIXME added to the code that I'll keep for now since changing it will spread outward and increase the patch size, I think.

Fixes #37074.

r? @jseyfried
cc @oberien

@Mark-Simulacrum Mark-Simulacrum added the relnotes Marks issues that should be documented in the release notes of the next release. label Jun 8, 2017
@Mark-Simulacrum Mark-Simulacrum changed the title Speed up expansion. Speed up expansion Jun 8, 2017
@Mark-Simulacrum Mark-Simulacrum force-pushed the macro-parse-speed-small branch from 9b86af6 to 1b6c57b Compare June 8, 2017 12:36
impl MatcherPos {
fn push_match(&mut self, idx: usize, m: NamedMatch) {
let matches = Rc::make_mut(&mut self.matches[idx]);
matches.push(m);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What a lovely hack 😻

This reduces duplication, thereby increasing expansion speed.
@Mark-Simulacrum Mark-Simulacrum force-pushed the macro-parse-speed-small branch from 1b6c57b to 3d9ebf2 Compare June 8, 2017 14:53
@Mark-Simulacrum
Copy link
Member Author

Pushed a theoretical fix for the run-pass-fulldeps test. I think it ran locally, but not sure. The full deps tests always get me.

@alexcrichton alexcrichton added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 8, 2017
@mattico
Copy link
Contributor

mattico commented Jun 8, 2017

cc meh/rust-uinput#1

@jseyfried
Copy link
Contributor

Excellent!
@bors r+

@bors
Copy link
Contributor

bors commented Jun 9, 2017

📌 Commit 3d9ebf2 has been approved by jseyfried

@bors
Copy link
Contributor

bors commented Jun 10, 2017

⌛ Testing commit 3d9ebf2 with merge d9d8f66...

@bors
Copy link
Contributor

bors commented Jun 10, 2017

💔 Test failed - status-appveyor

@est31
Copy link
Member

est31 commented Jun 10, 2017

@bors retry

Seems like a spurious issue.

thread '<unnamed>' panicked at 'assertion failed: a - Duration::new(0, 100) <= b', src\libstd\time\mod.rs:473
test time::tests::instant_math ... FAILED

@bors
Copy link
Contributor

bors commented Jun 10, 2017

⌛ Testing commit 3d9ebf2 with merge e148049...

bors added a commit that referenced this pull request Jun 10, 2017
…yfried

Speed up expansion

This reduces duplication, thereby increasing expansion speed. Based on tests with rust-uinput, this produces a 29x performance win (440 seconds to 15 seconds). I want to land this first, since it's a minimal patch, but with more changes to the macro parsing I can get down to 12 seconds locally.

There is one FIXME added to the code that I'll keep for now since changing it will spread outward and increase the patch size, I think.

Fixes #37074.

r? @jseyfried
cc @oberien
@bors
Copy link
Contributor

bors commented Jun 10, 2017

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

@bors bors merged commit 3d9ebf2 into rust-lang:master Jun 10, 2017
@Mark-Simulacrum Mark-Simulacrum deleted the macro-parse-speed-small branch June 10, 2017 14:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
relnotes Marks issues that should be documented in the release notes of the next release. S-waiting-on-review Status: Awaiting review from the assignee but also interested parties.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants