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

Remove `OpenSnapshot` and `CommittedSnapshot` markers. #14

Conversation

@nnethercote
Copy link
Contributor

commented Nov 5, 2018

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.

This commit removes them, and instead uses a counter to track how many
snapshots are open. One consequence of this is that violations of stack
discipline in API usage may be detected later -- e.g. if we rollback to
an earlier snapshot and then to a later snapshot, the code will panic on
the second rollback instead of the first. I think this is an acceptable
trade-off for the improved performance.

This commit also adds some basic unit tests.

Remove `OpenSnapshot` and `CommittedSnapshot` markers.
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`.

This commit removes them, and instead uses a counter to track how many
snapshots are open. One consequence of this is that violations of stack
discipline in API usage may be detected later -- e.g. if we rollback to
an earlier snapshot and then to a later snapshot, the code will panic on
the second rollback instead of the first. I think this is an acceptable
trade-off for the improved performance.

This commit also adds some basic unit tests.
@nnethercote

This comment has been minimized.

Copy link
Contributor Author

commented Nov 5, 2018

I removed these markers in ena and also in two places in rustc itself (librustc_data_structures/snapshot_map/mod.rs and librustc/infer/region_constraints/mod.rs). With all the changes in place, the instruction count results were remarkably good:

futures-check
	avg: -3.9%	min: -6.2%	max: -1.2%
packed_simd-check
	avg: -3.3%	min: -5.4%	max: -0.4%
serde-check
	avg: -2.7%	min: -3.7%	max: -0.8%
piston-image-check
	avg: -2.1%	min: -3.5%	max: -0.4%
crates.io-check
	avg: -2.1%	min: -3.3%	max: -0.5%
deep-vector-check
	avg: -2.1%	min: -3.2%	max: -0.1%
tokio-webpush-simple-check
	avg: -2.2%	min: -3.1%	max: -0.5%
tuple-stress-check
	avg: -1.8%	min: -3.0%	max: 0.3%
unify-linearly-check
	avg: -2.4%	min: -2.9%	max: -1.9%
ripgrep-check
	avg: -1.7%	min: -2.8%	max: -0.3%
regex-check
	avg: -1.2%	min: -2.8%	max: -0.4%
regression-31157-check
	avg: -2.0%	min: -2.7%	max: -1.0%
coercions-check
	avg: -1.4%?	min: -2.7%?	max: 0.4%?
webrender-check
	avg: -1.5%	min: -2.7%	max: -0.2%
sentry-cli-check
	avg: -1.5%	min: -2.4%	max: -0.2%
ucd-check
	avg: -1.5%	min: -2.4%	max: -0.5%
encoding-check
	avg: -1.5%	min: -2.3%	max: -0.5%
helloworld-check
	avg: -2.1%	min: -2.3%	max: -2.0%
syn-check
	avg: -1.5%	min: -2.3%	max: -0.4%
ctfe-stress-check
	avg: 0.1%?	min: -1.5%?	max: 2.2%?
cargo-check
	avg: -1.3%	min: -2.2%	max: -0.1%
style-servo-check
	avg: -1.4%	min: -2.0%	max: -0.1%
clap-rs-check
	avg: -1.4%	min: -2.0%	max: -0.8%
issue-46449-check
	avg: -1.7%	min: -1.9%	max: -1.4%
deeply-nested-check
	avg: -1.8%	min: -1.9%	max: -1.6%
html5ever-check
	avg: -1.1%	min: -1.9%	max: -0.0%
rust-unic-check
	avg: -1.0%	min: -1.2%	max: -0.9%
unused-warnings-check
	avg: -0.0%	min: -0.2%	max: 0.4%
inflate-check
	avg: -0.3%	min: -0.4%	max: -0.3%
keccak-check
	avg: -0.3%	min: -0.4%	max: -0.1%
@nnethercote

This comment has been minimized.

Copy link
Contributor Author

commented Nov 5, 2018

I guess a new version of ena would need to be published for rustc to be able to pick up these changes. @nikomatsakis: Should I bump the version number to 0.11.0 in a second patch?

@nnethercote

This comment has been minimized.

Copy link
Contributor Author

commented Nov 9, 2018

@nikomatsakis: any thoughts? This is part of a good-sized win.

@nnethercote

This comment has been minimized.

Copy link
Contributor Author

commented Nov 12, 2018

I removed these markers in ena and also in two places in rustc itself (librustc_data_structures/snapshot_map/mod.rs and librustc/infer/region_constraints/mod.rs).

The rustc changes are available in rust-lang/rust#55906.

@nikomatsakis

This comment has been minimized.

Copy link
Member

commented Nov 14, 2018

@nnethercote sorry, didn't see your pings till just now -- I'm 👍 on ena changes.

@nikomatsakis nikomatsakis merged commit f9c0061 into rust-lang-nursery:master Nov 14, 2018

1 check passed

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

nikomatsakis added a commit to nikomatsakis/ena that referenced this pull request Nov 14, 2018

bump version to 0.11.0
Landed PRs:

- optimize snapshots (rust-lang-nursery#14)
@nikomatsakis nikomatsakis referenced this pull request Nov 14, 2018

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

bors added a commit to rust-lang/rust 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 added a commit to rust-lang/rust 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 added a commit to rust-lang/rust that referenced this pull request Nov 24, 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 added a commit to rust-lang/rust that referenced this pull request Nov 24, 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 added a commit to rust-lang/rust that referenced this pull request Nov 24, 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 added a commit to rust-lang/rust 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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.