feat: Allow the UndoLog to be supplied separately #29
Conversation
From rust-lang/rust#69218 it was found that keeping an individual undo log for each type that need snapshotting incur noticeable overhead given how often snapshots are done and that they are in many cases empty. By separating the log like this, it is possible to store the undo log and storage separately and only put the together when acting the the underlying `UnificationTable` or `SnapshotVec`
Marwes
changed the title
There was a problem hiding this comment.
Ok, I did a first read. I am starting to understand what this PR is actually doing. =)
@Marwes is there any kind of write-up explaining the way this stuff is meant to be used?
ena is not really a very clearly encapsulated library, I forget if we have much in the way of docs -- if we did, I'd say you should definitely add to them. :P
Added a test showing a simple version, otherwise there is just rust-lang/rust#69464 |
|
@nikomatsakis Anything else I can do to move this forward? |
There was a problem hiding this comment.
Hi @Marwes -- sorry, it's been hard to carve out time, but I did a first pass over this change. I've asked for a lot of doc comments and the like, in part to make up for older code that should've been documented, but also to try and understand what the role is of each trait and so forth (to some extent, I'm "playing dumb" a bit here -- i.e., it's been a while since I looked at this code, so I have the advantage of being really confused and thus able to see better how much stuff is being assumed and not stated explicitly or commented).
src/unify/mod.rs
Outdated
| @@ -176,14 +180,21 @@ pub struct VarValue<K: UnifyKey> { // FIXME pub | |||
| /// - This implies that ordinary operations are quite a bit slower though. | |||
| /// - Requires the `persistent` feature be selected in your Cargo.toml file. | |||
| #[derive(Clone, Debug, Default)] | |||
| pub struct UnificationTable<S: UnificationStore> { | |||
| pub struct UnificationTable<S> { | |||
There was a problem hiding this comment.
Any reason to remove the bound here?
src/unify/mod.rs
Outdated
| K: UnifyKey, | ||
| V: sv::VecLike<Delegate<K>>, | ||
| { | ||
| pub fn with_log<'a, L2>( |
There was a problem hiding this comment.
Can you document how this method is meant to be used and what it does?
It seems a bit surprising to me since ordinarily I think of there being "just one" undo-log for a given table.
I guess the idea here is that, since snapshots are "stacked", you can "push a new log" on and you'll fully work through the snapshots that are in that new log before you come back to whatever log was there before?
There was a problem hiding this comment.
Changed this to only be implemented on the Storage types which has () in place of the undo log. Since () does not implement UndoLogs mutating methods can't be called. with_log creates an actual UnificationTable which can then be mutated.
src/undo_log.rs
Outdated
| fn actions_since_snapshot(&self, snapshot: &Self::Snapshot) -> &[T]; | ||
|
|
||
| fn start_snapshot(&mut self) -> Self::Snapshot; | ||
| fn rollback_to<R>(&mut self, values: impl FnOnce() -> R, snapshot: Self::Snapshot) |
There was a problem hiding this comment.
Rollback (undo) the changes made since the snapshot back.
Question:
What is values here?
src/undo_log.rs
Outdated
| } | ||
| } | ||
|
|
||
| /// A trait implemented for types which can be rolled back using actions of type `U`. |
There was a problem hiding this comment.
It'd be great to give an example of what is meant to implement Rollback -- I think it's the values in the table? Not sure
|
@Marwes I'll try to stay on top of this from now on! I want to see it land. Apologies for the delay. |
Not strictly necessary, but it documents things better and may make some mistakes impossible
|
Read through the latest changes. I'm going to merge this, thanks! |
|
@nikomatsakis Will update rust-lang/rust#69464 once a release of ena is done! |
Changes: * Allow the UndoLog to be supplied separately (rust-lang#29)
perf: Unify the undo log of all snapshot types Extracted from rust-lang#69218 and extended to all the current snapshot types. Since snapshotting is such a frequent action in the compiler and many of the scopes execute so little work, the act of creating the snapshot and rolling back empty/small snapshots end up showing in perf. By unifying all the logs into one the creation of snapshots becomes significantly cheaper at the cost of some complexity when combining the log with the specific data structures that are being mutated. Depends on rust-lang/ena#29
From rust-lang/rust#69218 it was found that
keeping an individual undo log for each type that need snapshotting
incur noticeable overhead given how often snapshots are done and that
they are in many cases empty.
By separating the log like this, it is possible to store the undo log
and storage separately and only put the together when acting the the
underlying
UnificationTableorSnapshotVec. In turn this allows multipleUnificationTables (and other types that can be snapshotted and reversed) to share a single undolog.cc rust-lang/rust#69464