-
Notifications
You must be signed in to change notification settings - Fork 34
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
Remove OpenSnapshot
and CommittedSnapshot
markers.
#14
Conversation
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.
I removed these markers in
|
I guess a new version of |
@nikomatsakis: any thoughts? This is part of a good-sized win. |
The rustc changes are available in rust-lang/rust#55906. |
@nnethercote sorry, didn't see your pings till just now -- I'm 👍 on ena changes. |
Landed PRs: - optimize snapshots (rust-lang#14)
… 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/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/ena#14 lands. But otherwise it should be good.
… 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/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/ena#14 lands. But otherwise it should be good.
When I recently searched for infos in this guide on how to create a warning, I couldn't find any. Later I found it through rust-lang#14. The reason was that I didn't know the term 'diagnostics' and that it is the collective term for errors, warnings and lints. Renaming the chapter to include the word 'error' should help. I think also including 'warning' in the title shouldn't be neccessary, because it's close enought.
They're not strictly necessary, and they result in the
Vec
beingallocated even for the trivial (and common) case where a
start_snapshot
is immediately followed by acommit
orrollback_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.