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

Use a SmallVec within _match::Matrix. #56269

Merged
merged 2 commits into from
Dec 10, 2018

Conversation

nnethercote
Copy link
Contributor

This avoids allocations.

@rust-highfive
Copy link
Collaborator

r? @davidtwco

(rust_highfive has picked a reviewer for you, use r? to override)

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

Note! This code doesn't compile currently. I get four compile errors:

error: unsatisfied lifetime constraints
   --> src/librustc_mir/hair/pattern/_match.rs:341:9
    |
317 |   impl<'a, 'tcx> MatchCheckCtxt<'a, 'tcx> {
    |        -- lifetime `'a` defined here
...
335 |       fn lower_byte_str_pattern<'p>(&mut self, pat: &'p Pattern<'tcx>)
    |                                 -- lifetime `'p` defined here
...
341 | /         self.byte_array_map.entry(pat).or_insert_with(|| {
342 | |             match pat.kind {
343 | |                 box PatternKind::Constant {
344 | |                     value: const_val
...   |
372 | |             }
373 | |         }).clone()
    | |__________________^ returning this value requires that `'p` must outlive `'a`

error[E0597]: `wild_patterns_owned` does not live long enough
    --> src/librustc_mir/hair/pattern/_match.rs:1254:33
     |
1254 |     let wild_patterns: Vec<_> = wild_patterns_owned.iter().collect();
     |                                 ^^^^^^^^^^^^^^^^^^^ borrowed value does not live long enough
...
1258 |     match specialize(cx, v, &ctor, &wild_patterns) {
     |           ---------------------------------------- a temporary with access to the borrow is created here ...
...
1269 | }
     | -
     | |
     | `wild_patterns_owned` dropped here while still borrowed
     | ... and the borrow might be used here, when that temporary is dropped and runs the destructor for type `std::option::Option<smallvec::SmallVec<[&hair::pattern::Pattern<'_>; 2]>>`
     |
     = note: The temporary is part of an expression at the end of a block. Consider forcing this temporary to be dropped sooner, before the block's local variables are dropped. For example, you could save the expression's value in a new local variable `x` and then make `x` be the expression at the end of the block.

error[E0597]: `wild_pattern` does not live long enough
   --> src/librustc_mir/hair/pattern/check_match.rs:287:56
    |
287 |             let witness = match is_useful(cx, &pats, &[&wild_pattern], ConstructWitness) {
    |                                                        ^^^^^^^^^^^^^ borrowed value does not live long enough
...
309 |         });
    |         -
    |         |
    |         `wild_pattern` dropped here while still borrowed
    |         borrow might be used here, when `pats` is dropped and runs the destructor for type `hair::pattern::_match::Matrix<'_, '_>`
    |
    = note: values in a scope are dropped in the opposite order they are defined

error[E0597]: `wild_pattern` does not live long enough
   --> src/librustc_mir/hair/pattern/check_match.rs:475:35
    |
466 | fn check_exhaustive<'a, 'tcx>(cx: &mut MatchCheckCtxt<'a, 'tcx>,
    |                     -- lifetime `'a` defined here
...
475 |     match is_useful(cx, matrix, &[&wild_pattern], ConstructWitness) {
    |           ------------------------^^^^^^^^^^^^^--------------------
    |           |                       |
    |           |                       borrowed value does not live long enough
    |           argument requires that `wild_pattern` is borrowed for `'a`
...
514 | }
    | - `wild_pattern` dropped here while still borrowed

This is at least partly because SmallVec doesn't use may_dangle. If I add that to SmallVec -- see servo/rust-smallvec#132 -- it avoids the 2nd and 3rd error, but the 1st and 4th remain. So either the way I've added may_dangle isn't right, or there's something else going on as well.

@nikomatsakis: this is similar to #55525.

@Mark-Simulacrum
Copy link
Member

r=me (@bors delegate+) -- one thing though might be to change the repeated SmallVec writing to a type alias placed somewhere, but since I didn't see a great place for it I don't think we should block on it or do it all perhaps

@rust-highfive

This comment has been minimized.

@nnethercote
Copy link
Contributor Author

@bors r-

As mentioned above, this code doesn't currently compile. I'm posting it to a PR because it's the easiest way to get others to look at it :)

one thing though might be to change the repeated SmallVec writing to a type alias placed somewhere

I tried that, and ended up with this:

type PatternVec<'a, 'tcx> = SmallVec<[&'a Pattern<'tcx>; 2]>>;

and when you see a PatternVec in isolation it's not clear what the two lifetimes are referring to, i.e. I felt that it obscured things. But I could be convinced otherwise.

@bors bors 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 27, 2018
@Mark-Simulacrum
Copy link
Member

That's a good point -- though PatternRefVec<'a, 'tcx> might be a bit clearer.

Also, r=me doesn't actually bors-approve, I just indicated that the overall change seemed good so when ready you can go ahead and r=me :)

@nnethercote
Copy link
Contributor Author

nnethercote commented Nov 29, 2018

I put new code up. If works if SmallVec gets may_dangle added to it. I.e. this is now only blocked by servo/rust-smallvec#132.

@rust-highfive

This comment has been minimized.

@nnethercote
Copy link
Contributor Author

smallvec 0.6.7 now has may_dangle support, and this change is ok to land.

@Mark-Simulacrum: want to take another look before landing?

@Mark-Simulacrum
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Nov 30, 2018

📌 Commit 123df632f43fd20c57badf1d591bf11a4bdc044f has been approved by Mark-Simulacrum

@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 Nov 30, 2018
@bors
Copy link
Contributor

bors commented Dec 2, 2018

🔒 Merge conflict

This pull request and the master branch diverged in a way that cannot be automatically merged. Please rebase on top of the latest master branch, and let the reviewer approve again.

How do I rebase?

Assuming self is your fork and upstream is this repository, you can resolve the conflict following these steps:

  1. git checkout _match-Matrix-SmallVec (switch to your branch)
  2. git fetch upstream master (retrieve the latest master)
  3. git rebase upstream/master -p (rebase on top of it)
  4. Follow the on-screen instruction to resolve conflicts (check git status if you got lost).
  5. git push self _match-Matrix-SmallVec --force-with-lease (update this PR)

You may also read Git Rebasing to Resolve Conflicts by Drew Blessing for a short tutorial.

Please avoid the "Resolve conflicts" button on GitHub. It uses git merge instead of git rebase which makes the PR commit history more difficult to read.

Sometimes step 4 will complete without asking for resolution. This is usually due to difference between how Cargo.lock conflict is handled during merge and rebase. This is normal, and you should still perform step 5 to update this PR.

Error message
warning: Cannot merge binary files: Cargo.lock (HEAD vs. heads/homu-tmp)
Auto-merging src/libsyntax_ext/Cargo.toml
Auto-merging src/librustc_mir/hair/pattern/check_match.rs
Auto-merging src/librustc/Cargo.toml
Auto-merging Cargo.lock
CONFLICT (content): Merge conflict in Cargo.lock
Automatic merge failed; fix conflicts and then commit the result.

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Dec 2, 2018
@bors
Copy link
Contributor

bors commented Dec 5, 2018

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

@nnethercote
Copy link
Contributor Author

I have rebased.

@bors r=simulacrum

@bors
Copy link
Contributor

bors commented Dec 9, 2018

📌 Commit 642ad4221857731e6cc170431a8bb2ed1f6bb055 has been approved by simulacrum

@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 Dec 9, 2018
@rust-highfive
Copy link
Collaborator

The job x86_64-gnu-llvm-5.0 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:04144704:start=1544394824014681787,finish=1544394826168454948,duration=2153773161
$ 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=x86_64-gnu-llvm-5.0
---
[00:12:02]     --> src/librustc_mir/hair/pattern/_match.rs:1672:13
[00:12:02]      |
[00:12:02] 1672 | /             match *constructor {
[00:12:02] 1673 | |                 Slice(..) => {
[00:12:02] 1674 | |                     // we extract an `Option` for the pointer because slices of zero elements don't
[00:12:02] 1675 | |                     // necessarily point to memory, they are usually just integers. The only time
[00:12:02] 1738 | |                 }
[00:12:02] 1739 | |             }
[00:12:02]      | |_____________^ expected struct `std::vec::Vec`, found struct `smallvec::SmallVec`
[00:12:02]      |
[00:12:02]      |
[00:12:02]      = note: expected type `std::option::Option<std::vec::Vec<&hair::pattern::Pattern<'_>>>`
[00:12:02]                 found type `std::option::Option<smallvec::SmallVec<[&hair::pattern::Pattern<'_>; 2]>>`
[00:12:02] note: match arm with an incompatible type
[00:12:02]      |
[00:12:02] 1733 |                   _ => {
[00:12:02]      |  ______________________^
[00:12:02] 1734 | |                     // If the constructor is a:
[00:12:02] 1734 | |                     // If the constructor is a:
[00:12:02] 1735 | |                     //      Single value: add a row if the constructor equals the pattern.
[00:12:02] 1736 | |                     //      Range: add a row if the constructor contains the pattern.
[00:12:02] 1737 | |                     constructor_intersects_pattern(cx.tcx, constructor, pat)
[00:12:02]      | |_________________^
[00:12:02] 
[00:12:08] error: aborting due to previous error
[00:12:08] 
---
[00:13:35] command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "build" "--target" "x86_64-unknown-linux-gnu" "-j" "4" "--release" "--locked" "--color" "always" "--features" "" "--manifest-path" "/checkout/src/rustc/Cargo.toml" "--message-format" "json"
[00:13:35] expected success, got: exit code: 101
[00:13:35] failed to run: /checkout/obj/build/bootstrap/debug/bootstrap build
[00:13:35] Build completed unsuccessfully in 0:10:38
[00:13:35] make: *** [all] Error 1
[00:13:35] Makefile:28: recipe for target 'all' failed

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)

@Mark-Simulacrum
Copy link
Member

@bors r-

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Dec 9, 2018
This commit also fixes up lifetimes a bit:

- Renames `'a` as `'p` when used with `Matrix` and `Pattern`, for
  consistency.

- Removes some unnecessary `'p` lifetimes on some function arguments.

- Adds some missing lifetime parameters.
@nnethercote
Copy link
Contributor Author

I have updated. Just required changing a Vec::new to SmallVec::new.

@bors r=simulacrum

@bors
Copy link
Contributor

bors commented Dec 10, 2018

📌 Commit cdc6633 has been approved by simulacrum

@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 Dec 10, 2018
@bors
Copy link
Contributor

bors commented Dec 10, 2018

⌛ Testing commit cdc6633 with merge e2c329c...

bors added a commit that referenced this pull request Dec 10, 2018
Use a `SmallVec` within `_match::Matrix`.

This avoids allocations.
@bors
Copy link
Contributor

bors commented Dec 10, 2018

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

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.

5 participants