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 item_bodies for large match statements involving regions #57494

Merged
merged 2 commits into from
Jan 13, 2019

Conversation

dotdash
Copy link
Contributor

@dotdash dotdash commented Jan 10, 2019

These changes don't change anything about the complexity of the algorithms, but use some easy shortcuts or modifications to cut down some overhead.

The first change, which incrementally removes the constraints from the set we're iterating over probably introduces some overhead for small to medium sized constraint sets, but it's not big enough for me to observe it in any project I tested against (not that many though).

Though most other crates probably won't improve much at all, because huge matches aren't that common, the changes seemed simple enough for me to make them.

Ref #55528

cc unicode-rs/unicode-normalization#29

r? @nikomatsakis

Once a region has been expanded to cover a fixed region, a corresponding
RegSubVar constraint won't have any effect on the expansion anymore, the
same is true for constraints where the variable on the RHS has already
reached static scope. By removing those constraints from the set that
we're iterating over, we remove a lot of needless overhead in case of
slow convergences (i.e. lots of iterations).

For the unicode_normalization crate, this about cuts the time required
for item_bodies checking in half.
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 10, 2019
@rust-highfive

This comment has been minimized.

In functions with lots of region constraint, if the fixed point
iteration converges only slowly, a lot of the var/var constraints will
have equal regions most of the time. Yet, we still perform the LUB
calculation and try to intern the result. Especially the latter incurs
quite some overhead.

This reduces the take taken by the item bodies checking pass for the
unicode_normalization crate by about 75%.
@dotdash dotdash changed the title Speed up item_bodies and match check for large match statements Speed up item_bodies for large match statements involving regions Jan 10, 2019
@dotdash
Copy link
Contributor Author

dotdash commented Jan 10, 2019

Removed the commit that modifies the match checking code for now, as that caused a test failure.

@tesuji
Copy link
Contributor

tesuji commented Jan 10, 2019

@bors try

@bors
Copy link
Contributor

bors commented Jan 10, 2019

@lzutao: 🔑 Insufficient privileges: not in try users

@Mark-Simulacrum
Copy link
Member

@bors try

@bors
Copy link
Contributor

bors commented Jan 10, 2019

⌛ Trying commit 5f402b8 with merge 960c730...

bors added a commit that referenced this pull request Jan 10, 2019
Speed up item_bodies for large match statements involving regions

These changes don't change anything about the complexity of the algorithms, but use some easy shortcuts or modifications to cut down some overhead.

The first change, which incrementally removes the constraints from the set we're iterating over probably introduces some overhead for small to medium sized constraint sets, but it's not big enough for me to observe it in any project I tested against (not that many though).

Though most other crates probably won't improve much at all, because huge matches aren't that common, the changes seemed simple enough for me to make them.

Ref #55528

cc unicode-rs/unicode-normalization#29

r? @nikomatsakis
@bors
Copy link
Contributor

bors commented Jan 11, 2019

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

@tesuji
Copy link
Contributor

tesuji commented Jan 11, 2019

@rust-timer build 960c730

@rust-timer
Copy link
Collaborator

Insufficient permissions to issue commands to rust-timer.

@dotdash
Copy link
Contributor Author

dotdash commented Jan 11, 2019

@rust-timer build 960c730

@rust-timer
Copy link
Collaborator

Insufficient permissions to issue commands to rust-timer.

@nagisa
Copy link
Member

nagisa commented Jan 11, 2019

@rust-timer build 960c730

@rust-timer
Copy link
Collaborator

Success: Queued 960c730 with parent 6ecad33, comparison URL.

@rust-timer
Copy link
Collaborator

Finished benchmarking try commit 960c730

@nikomatsakis
Copy link
Contributor

@bors r+

@bors
Copy link
Contributor

bors commented Jan 11, 2019

📌 Commit 5f402b8 has been approved by nikomatsakis

@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 11, 2019
pietroalbini added a commit to pietroalbini/rust that referenced this pull request Jan 12, 2019
Speed up item_bodies for large match statements involving regions

These changes don't change anything about the complexity of the algorithms, but use some easy shortcuts or modifications to cut down some overhead.

The first change, which incrementally removes the constraints from the set we're iterating over probably introduces some overhead for small to medium sized constraint sets, but it's not big enough for me to observe it in any project I tested against (not that many though).

Though most other crates probably won't improve much at all, because huge matches aren't that common, the changes seemed simple enough for me to make them.

Ref rust-lang#55528

cc unicode-rs/unicode-normalization#29

r? @nikomatsakis
Centril added a commit to Centril/rust that referenced this pull request Jan 13, 2019
Speed up item_bodies for large match statements involving regions

These changes don't change anything about the complexity of the algorithms, but use some easy shortcuts or modifications to cut down some overhead.

The first change, which incrementally removes the constraints from the set we're iterating over probably introduces some overhead for small to medium sized constraint sets, but it's not big enough for me to observe it in any project I tested against (not that many though).

Though most other crates probably won't improve much at all, because huge matches aren't that common, the changes seemed simple enough for me to make them.

Ref rust-lang#55528

cc unicode-rs/unicode-normalization#29

r? @nikomatsakis
bors added a commit that referenced this pull request Jan 13, 2019
Rollup of 16 pull requests

Successful merges:

 - #57351 (Don't actually create a full MIR stack frame when not needed)
 - #57353 (Optimise floating point `is_finite` (2x) and `is_infinite` (1.6x).)
 - #57412 (Improve the wording)
 - #57436 (save-analysis: use a fallback when access levels couldn't be computed)
 - #57453 (lldb_batchmode.py: try `import _thread` for Python 3)
 - #57454 (Some cleanups for core::fmt)
 - #57461 (Change `String` to `&'static str` in `ParseResult::Failure`.)
 - #57473 (std: Render large exit codes as hex on Windows)
 - #57474 (save-analysis: Get path def from parent in case there's no def for the path itself.)
 - #57494 (Speed up item_bodies for large match statements involving regions)
 - #57496 (re-do docs for core::cmp)
 - #57508 (rustdoc: Allow inlining of reexported crates and crate items)
 - #57547 (Use `ptr::eq` where applicable)
 - #57557 (resolve: Mark extern crate items as used in more cases)
 - #57560 (hygiene: Do not treat `Self` ctor as a local variable)
 - #57564 (Update the const fn tracking issue to the new metabug)

Failed merges:

r? @ghost
@bors bors merged commit 5f402b8 into rust-lang:master Jan 13, 2019
@dotdash dotdash deleted the expand branch January 20, 2020 08:56
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