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

Clean up and streamline snapshot data structures #55906

Merged
merged 8 commits into from Nov 25, 2018

Conversation

Projects
None yet
8 participants
@nnethercote
Contributor

nnethercote commented Nov 12, 2018

These commits clean up the snapshot structures a bit, so they are more consistent with each other and with the ena crate.

They also remove the OpenSnapshot and CommittedSnapshot entries in the undo log, just like I did for the ena crate in rust-lang-nursery/ena#14. This PR in combination with that ena PR reduces instruction counts by up to 6% on benchmarks.

r? @nikomatsakis. Note that this isn't quite ready for landing, because the ena dependency in the first commit needs to be updated once rust-lang-nursery/ena#14 lands. But otherwise it should be good.

@rust-highfive

This comment has been minimized.

Collaborator

rust-highfive commented Nov 12, 2018

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:0095aeb8:start=1542063484397050670,finish=1542063545869501563,duration=61472450893
$ 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:02:04] 
################################                                          44.9%
######################################################################## 100.0%
[00:02:04] extracting /checkout/obj/build/cache/2018-10-30/cargo-beta-x86_64-unknown-linux-gnu.tar.gz
[00:02:04] error: failed to read `/ena2/Cargo.toml`
[00:02:04] Caused by:
[00:02:04]   No such file or directory (os error 2)
[00:02:04] failed to run: /checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo build --manifest-path /checkout/src/bootstrap/Cargo.toml --locked
[00:02:04] Build completed unsuccessfully in 0:00:14
[00:02:04] Build completed unsuccessfully in 0:00:14
[00:02:04] Makefile:81: recipe for target 'prepare' failed
[00:02:04] make: *** [prepare] Error 1
[00:02:05] Command failed. Attempt 2/5:
[00:02:05] error: failed to read `/ena2/Cargo.toml`
[00:02:05] Caused by:
[00:02:05]   No such file or directory (os error 2)
[00:02:05] failed to run: /checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo build --manifest-path /checkout/src/bootstrap/Cargo.toml --locked
[00:02:05] Build completed unsuccessfully in 0:00:00
[00:02:05] Build completed unsuccessfully in 0:00:00
[00:02:05] make: *** [prepare] Error 1
[00:02:05] Makefile:81: recipe for target 'prepare' failed
[00:02:07] Command failed. Attempt 3/5:
[00:02:07] error: failed to read `/ena2/Cargo.toml`
[00:02:07] Caused by:
[00:02:07]   No such file or directory (os error 2)
[00:02:07] failed to run: /checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo build --manifest-path /checkout/src/bootstrap/Cargo.toml --locked
[00:02:07] Build completed unsuccessfully in 0:00:00
[00:02:07] Build completed unsuccessfully in 0:00:00
[00:02:07] Makefile:81: recipe for target 'prepare' failed
[00:02:07] make: *** [prepare] Error 1
[00:02:10] Command failed. Attempt 4/5:
[00:02:10] error: failed to read `/ena2/Cargo.toml`
[00:02:10] Caused by:
[00:02:10]   No such file or directory (os error 2)
[00:02:10] failed to run: /checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo build --manifest-path /checkout/src/bootstrap/Cargo.toml --locked
[00:02:10] Build completed unsuccessfully in 0:00:00

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)

@nikomatsakis

This comment has been minimized.

Contributor

nikomatsakis commented Nov 14, 2018

@nnethercote nnethercote force-pushed the nnethercote:rm-OpenSnapshot-CommittedSnapshot branch from c189efa to 350a15a Nov 15, 2018

@nnethercote

This comment has been minimized.

Contributor

nnethercote commented Nov 15, 2018

I've updated the first commit so that it uses ena 0.11.0. I confirmed that I'm still getting the speed-ups of up to 6%.

@nikomatsakis

This comment has been minimized.

Contributor

nikomatsakis commented Nov 16, 2018

@bors r+

@bors

This comment has been minimized.

Contributor

bors commented Nov 16, 2018

📌 Commit 350a15a has been approved by nikomatsakis

@nnethercote

This comment has been minimized.

Contributor

nnethercote commented Nov 20, 2018

@bors try

@bors

This comment has been minimized.

Contributor

bors commented Nov 20, 2018

🙅 Please do not try after a pull request has been r+ed. If you need to try, unapprove (r-) it first.

@bors

This comment has been minimized.

Contributor

bors commented Nov 21, 2018

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

@nnethercote nnethercote force-pushed the nnethercote:rm-OpenSnapshot-CommittedSnapshot branch from 350a15a to bf614a8 Nov 21, 2018

@nnethercote

This comment has been minimized.

Contributor

nnethercote commented Nov 21, 2018

@bors try

@bors

This comment has been minimized.

Contributor

bors commented Nov 21, 2018

⌛️ Trying commit bf614a8 with merge 04eac86...

bors added a commit that referenced this pull request Nov 21, 2018

Auto merge of #55906 - nnethercote:rm-OpenSnapshot-CommittedSnapshot,…
… r=<try>

Clean up and streamline snapshot data structures

These commits clean up the snapshot structures a bit, so they are more consistent with each other and with the `ena` crate.

They also remove the `OpenSnapshot` and `CommittedSnapshot` entries in the undo log, just like I did for the `ena` crate in rust-lang-nursery/ena#14. This PR in combination with that `ena` PR reduces instruction counts by up to 6% on benchmarks.

r? @nikomatsakis. Note that this isn't quite ready for landing, because the `ena` dependency in the first commit needs to be updated once rust-lang-nursery/ena#14 lands. But otherwise it should be good.
@rust-highfive

This comment has been minimized.

Collaborator

rust-highfive commented Nov 21, 2018

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:01af613e:start=1542840983454530064,finish=1542840986009267361,duration=2554737297
$ 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
---
######################################################################    97.8%
######################################################################## 100.0%
[00:02:11] extracting /checkout/obj/build/cache/2018-10-30/cargo-beta-x86_64-unknown-linux-gnu.tar.gz
[00:02:11]     Updating crates.io index
[00:02:18] error: the lock file /checkout/src/Cargo.lock needs to be updated but --locked was passed to prevent this
[00:02:18] Build completed unsuccessfully in 0:00:23
[00:02:18] Makefile:81: recipe for target 'prepare' failed
[00:02:18] make: *** [prepare] Error 1
[00:02:19] Command failed. Attempt 2/5:
[00:02:19] Command failed. Attempt 2/5:
[00:02:19]     Updating crates.io index
[00:02:19] error: the lock file /checkout/src/Cargo.lock needs to be updated but --locked was passed to prevent this
[00:02:19] Build completed unsuccessfully in 0:00:00
[00:02:19] make: *** [prepare] Error 1
[00:02:19] Makefile:81: recipe for target 'prepare' failed
[00:02:21] Command failed. Attempt 3/5:
[00:02:21] Command failed. Attempt 3/5:
[00:02:21]     Updating crates.io index
[00:02:22] error: the lock file /checkout/src/Cargo.lock needs to be updated but --locked was passed to prevent this
[00:02:22] Build completed unsuccessfully in 0:00:00
[00:02:22] make: *** [prepare] Error 1
[00:02:22] Makefile:81: recipe for target 'prepare' failed
[00:02:25] Command failed. Attempt 4/5:
[00:02:25] Command failed. Attempt 4/5:
[00:02:25]     Updating crates.io index
[00:02:25] error: the lock file /checkout/src/Cargo.lock needs to be updated but --locked was passed to prevent this
[00:02:25] Build completed unsuccessfully in 0:00:00
[00:02:25] make: *** [prepare] Error 1
[00:02:25] Makefile:81: recipe for target 'prepare' failed
[00:02:25] Makefile:81: recipe for target 'prepare' failed
[00:02:30] error: the lock file /checkout/src/Cargo.lock needs to be updated but --locked was passed to prevent this
[00:02:30] Build completed unsuccessfully in 0:00:00
[00:02:30] Makefile:81: recipe for target 'prepare' failed
[00:02:30] make: *** [prepare] Error 1
[00:02:30] The command has failed after 5 attempts.

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)

@bors

This comment has been minimized.

Contributor

bors commented Nov 21, 2018

💔 Test failed - status-travis

@rust-highfive

This comment has been minimized.

Collaborator

rust-highfive commented Nov 21, 2018

The job dist-x86_64-linux 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_fold:end:services

travis_fold:start:git.checkout
travis_time:start:2c771ba4
$ git clone --depth=2 --branch=try https://github.com/rust-lang/rust.git rust-lang/rust
---
###                                                                        5.3%
######################################################################## 100.0%
[00:04:16] extracting /checkout/obj/build/cache/2018-10-30/cargo-beta-x86_64-unknown-linux-gnu.tar.gz
[00:04:16]     Updating crates.io index
[00:04:22] error: the lock file /checkout/src/Cargo.lock needs to be updated but --locked was passed to prevent this
[00:04:22] Build completed unsuccessfully in 0:00:23
[00:04:22] make: *** [prepare] Error 1
[00:04:23] Command failed. Attempt 2/5:
[00:04:23]     Updating crates.io index
[00:04:23]     Updating crates.io index
[00:04:24] error: the lock file /checkout/src/Cargo.lock needs to be updated but --locked was passed to prevent this
[00:04:24] Build completed unsuccessfully in 0:00:00
[00:04:24] make: *** [prepare] Error 1
[00:04:26] Command failed. Attempt 3/5:
[00:04:26]     Updating crates.io index
[00:04:26]     Updating crates.io index
[00:04:26] error: the lock file /checkout/src/Cargo.lock needs to be updated but --locked was passed to prevent this
[00:04:26] Build completed unsuccessfully in 0:00:00
[00:04:26] make: *** [prepare] Error 1
[00:04:29] Command failed. Attempt 4/5:
[00:04:30]     Updating crates.io index
[00:04:30]     Updating crates.io index
[00:04:30] error: the lock file /checkout/src/Cargo.lock needs to be updated but --locked was passed to prevent this
[00:04:30] Build completed unsuccessfully in 0:00:00
[00:04:30] make: *** [prepare] Error 1
          768M     0  768M   0% /var/ramfs
2616972 .

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)

@nnethercote nnethercote force-pushed the nnethercote:rm-OpenSnapshot-CommittedSnapshot branch from bf614a8 to f61f9ed Nov 21, 2018

@nnethercote

This comment has been minimized.

Contributor

nnethercote commented Nov 21, 2018

@bors try

@bors

This comment has been minimized.

Contributor

bors commented Nov 21, 2018

⌛️ Trying commit f61f9ed with merge 2f28ba5...

bors added a commit that referenced this pull request Nov 21, 2018

Auto merge of #55906 - nnethercote:rm-OpenSnapshot-CommittedSnapshot,…
… r=<try>

Clean up and streamline snapshot data structures

These commits clean up the snapshot structures a bit, so they are more consistent with each other and with the `ena` crate.

They also remove the `OpenSnapshot` and `CommittedSnapshot` entries in the undo log, just like I did for the `ena` crate in rust-lang-nursery/ena#14. This PR in combination with that `ena` PR reduces instruction counts by up to 6% on benchmarks.

r? @nikomatsakis. Note that this isn't quite ready for landing, because the `ena` dependency in the first commit needs to be updated once rust-lang-nursery/ena#14 lands. But otherwise it should be good.
@bors

This comment has been minimized.

Contributor

bors commented Nov 22, 2018

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

@nnethercote

This comment has been minimized.

Contributor

nnethercote commented Nov 22, 2018

@bors

This comment has been minimized.

Contributor

bors commented Nov 25, 2018

💡 This pull request was already approved, no need to approve it again.

  • This pull request previously failed. You should add more commits to fix the bug, or use retry to trigger a build again.
  • There's another pull request that is currently being tested, blocking this pull request: #49219
@bors

This comment has been minimized.

Contributor

bors commented Nov 25, 2018

📌 Commit bfc1902 has been approved by nikomatsakis

@bors

This comment has been minimized.

Contributor

bors commented Nov 25, 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 rm-OpenSnapshot-CommittedSnapshot (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 rm-OpenSnapshot-CommittedSnapshot --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 Cargo.lock
CONFLICT (content): Merge conflict in Cargo.lock
Automatic merge failed; fix conflicts and then commit the result.

nnethercote added some commits Nov 2, 2018

Update to `ena` 0.11.0.
This version has some significant speed-ups relating to snapshotting.
Remove `insert_noop`.
Because it's as useless as its name suggests.

This commit also renames `UndoLog::Noop` as `UndoLog::Purged`, because
(a) that's a more descriptive name and (b) it matches the name used in
similar code in `librustc/infer/region_constraints/mod.rs`.
Rename `UndoLogEntry` as `UndoLog`.
So that it matches `librustc_data_structures/snapshot_map/mod.rs` and
the `ena` crate.
Make `commit` and `rollback_to` methods take ownership of the snapshots.
Because they shouldn't be reused. This provides consistency with the
`ena` crate.
Introduce `in_snapshot` and `assert_open_snapshot` methods.
This makes the two snapshot implementations more consistent with each
other and with crate `ena`.
Remove `OpenSnapshot` and `CommittedSnapshot` markers from `SnapshotM…
…ap`.

They're not strictly necessary, and they result in the `Vec` being
allocated even for the trivial (and common) case where a
`start_snapshot` is immediately followed by a `commit` or `rollback_to`.
Remove `OpenSnapshot` and `CommittedSnapshot` markers from `RegionCon…
…straintCollector`.

They're not strictly necessary, and they result in the `Vec` being
allocated even for the trivial (and common) case where a
`start_snapshot` is immediately followed by a `commit` or `rollback_to`.

The commit also removes a now-unnecessary argument of
`pop_placeholders()`.

@nnethercote nnethercote force-pushed the nnethercote:rm-OpenSnapshot-CommittedSnapshot branch from bfc1902 to 94967ae Nov 25, 2018

@nnethercote

This comment has been minimized.

Contributor

nnethercote commented Nov 25, 2018

@bors r=nikomatsakis

@bors

This comment has been minimized.

Contributor

bors commented Nov 25, 2018

📌 Commit 94967ae has been approved by nikomatsakis

@bors

This comment has been minimized.

Contributor

bors commented Nov 25, 2018

⌛️ Testing commit 94967ae with merge e9bca7a...

bors added a commit that referenced this pull request Nov 25, 2018

Auto merge of #55906 - nnethercote:rm-OpenSnapshot-CommittedSnapshot,…
… r=nikomatsakis

Clean up and streamline snapshot data structures

These commits clean up the snapshot structures a bit, so they are more consistent with each other and with the `ena` crate.

They also remove the `OpenSnapshot` and `CommittedSnapshot` entries in the undo log, just like I did for the `ena` crate in rust-lang-nursery/ena#14. This PR in combination with that `ena` PR reduces instruction counts by up to 6% on benchmarks.

r? @nikomatsakis. Note that this isn't quite ready for landing, because the `ena` dependency in the first commit needs to be updated once rust-lang-nursery/ena#14 lands. But otherwise it should be good.
@bors

This comment has been minimized.

Contributor

bors commented Nov 25, 2018

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

@bors bors merged commit 94967ae into rust-lang:master Nov 25, 2018

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details

@nnethercote nnethercote deleted the nnethercote:rm-OpenSnapshot-CommittedSnapshot branch Nov 25, 2018

@nnethercote

This comment has been minimized.

Contributor

nnethercote commented Nov 25, 2018

Just for posterity: this PR finally landed 8 days and 19 hours after the initial r+. I'm sure that's the slowest landing I've ever had. It was in the queue for almost all that time, mostly hovering around positions 6, 7 and 8, continually being bumped. I had to rebase it four or five times, and for one of those rebases I wasn't quick enough and so I missed an opening for landing, which delayed things by few hours more.

@Centril

This comment has been minimized.

Contributor

Centril commented Nov 28, 2018

cc @kennytm re. @nnethercote's previous comment... what's up with that?

@petrochenkov

This comment has been minimized.

Contributor

petrochenkov commented Nov 28, 2018

First, prioritized edition-critical PRs are flying around between master and beta right now.
Second, rollups of small PRs are prioritized.
This PR is neither critical nor small enough to rollup - the saddest kind of PR to land.

@nnethercote

This comment has been minimized.

Contributor

nnethercote commented Nov 28, 2018

Landing cycle time now exceeds 3 hours, which means there are about 7.5 slots per 24 hours. Combine that with the fact that it feels like intermittent failures are more frequent lately, and the number of successful landings per 24 hours is probably something like 5.

@nnethercote

This comment has been minimized.

Contributor

nnethercote commented Nov 29, 2018

Final perf results -- wins of up to 5%, with almost every benchmark seeing at least some improvement.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment