feat: Allow the UndoLog to be supplied separately #29

Marwes · 2020-02-26T09:53:18Z

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. In turn this allows multiple UnificationTables (and other types that can be snapshotted and reversed) to share a single undolog.

cc rust-lang/rust#69464

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`

…otting

nikomatsakis

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

src/unify/mod.rs

Marwes · 2020-03-20T10:23:01Z

is there any kind of write-up explaining the way this stuff is meant to be used?

Added a test showing a simple version, otherwise there is just rust-lang/rust#69464

Marwes · 2020-03-27T10:13:52Z

@nikomatsakis Anything else I can do to move this forward?

nikomatsakis

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/undo_log.rs

nikomatsakis · 2020-04-06T20:39:00Z

src/unify/mod.rs

@@ -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> {


Any reason to remove the bound here?

nikomatsakis · 2020-04-06T20:41:09Z

src/unify/mod.rs

+    K: UnifyKey,
+    V: sv::VecLike<Delegate<K>>,
+{
+    pub fn with_log<'a, L2>(


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?

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/unify/tests.rs

src/undo_log.rs

nikomatsakis · 2020-04-06T20:52:43Z

src/undo_log.rs

+    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)


Rollback (undo) the changes made since the snapshot back.

Question:

What is values here?

nikomatsakis · 2020-04-06T20:53:49Z

src/undo_log.rs

+    }
+}
+
+/// A trait implemented for types which can be rolled back using actions of type `U`.


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

nikomatsakis · 2020-04-06T20:55:57Z

@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

nikomatsakis · 2020-04-09T23:21:40Z

Read through the latest changes. I'm going to merge this, thanks!

Marwes · 2020-04-10T10:22:48Z

@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

Markus Westerlind added 8 commits February 25, 2020 17:27

Prevent 'Storage' types from being modified without a log

f10d1d3

Add UNificationStorage back

bd84def

Allow Rollback on the wrapper storage types

4b08d1c

Relax UnificationTable::new

1f370f2

Don't require UndoLogs for calling len

528af9a

Fix persistent tables

25f94c2

Split UnificationStore into a Mut variant which do not require snapsh…

30daa1d

…otting

Marwes mentioned this pull request Feb 26, 2020

perf: Unify the undo log of all snapshot types rust-lang/rust#69464

Merged

Marwes changed the title ~~feat: Allow the UndoLog to supplied separately~~ feat: Allow the UndoLog to be supplied separately Feb 26, 2020

Marwes mentioned this pull request Mar 2, 2020

perf: Only process changed obligations in ObligationForest rust-lang/rust#69218

Closed

Add some documentation

ffbc9c3

nikomatsakis self-assigned this Mar 3, 2020

nikomatsakis reviewed Mar 19, 2020

View reviewed changes

src/unify/mod.rs Outdated Show resolved Hide resolved

Markus Westerlind added 2 commits March 20, 2020 10:43

docL Document the undo_log module

5ae2cfa

test: Check the unified undo log addition

fac8c2d

nikomatsakis requested changes Apr 6, 2020

View reviewed changes

Markus Westerlind added 7 commits April 7, 2020 10:24

Fix review comments

ee9764b

Fix review comments

7d7d5e9

Fix review comments

beeafed

Fix review comments

9568b3b

refactor: Restrict with_log to the Storage types

b1934a1

Not strictly necessary, but it documents things better and may make some mistakes impossible

refactor: Clarify reverse forwarding

c0ab90a

doc: Explain the opaqueness of T in UndoLogs<T>

7e3b49b

nikomatsakis merged commit 5a500c4 into rust-lang:master Apr 9, 2020

Marwes deleted the detach_undo_log branch April 10, 2020 06:41

nikomatsakis added a commit to nikomatsakis/ena that referenced this pull request Apr 10, 2020

release 0.14.0

fb6143a

Changes: * Allow the UndoLog to be supplied separately (rust-lang#29)

nikomatsakis mentioned this pull request Apr 10, 2020

release 0.14.0 #31

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Allow the UndoLog to be supplied separately #29

feat: Allow the UndoLog to be supplied separately #29

Marwes commented Feb 26, 2020 •

edited

Loading

nikomatsakis left a comment

Marwes commented Mar 20, 2020

Marwes commented Mar 27, 2020

nikomatsakis left a comment

nikomatsakis Apr 6, 2020

nikomatsakis Apr 6, 2020

Marwes Apr 7, 2020

nikomatsakis Apr 6, 2020

nikomatsakis Apr 6, 2020

nikomatsakis commented Apr 6, 2020

nikomatsakis commented Apr 9, 2020

Marwes commented Apr 10, 2020

feat: Allow the UndoLog to be supplied separately #29

feat: Allow the UndoLog to be supplied separately #29

Conversation

Marwes commented Feb 26, 2020 • edited Loading

nikomatsakis left a comment

Choose a reason for hiding this comment

Marwes commented Mar 20, 2020

Marwes commented Mar 27, 2020

nikomatsakis left a comment

Choose a reason for hiding this comment

nikomatsakis Apr 6, 2020

Choose a reason for hiding this comment

nikomatsakis Apr 6, 2020

Choose a reason for hiding this comment

Marwes Apr 7, 2020

Choose a reason for hiding this comment

nikomatsakis Apr 6, 2020

Choose a reason for hiding this comment

nikomatsakis Apr 6, 2020

Choose a reason for hiding this comment

nikomatsakis commented Apr 6, 2020

nikomatsakis commented Apr 9, 2020

Marwes commented Apr 10, 2020

Marwes commented Feb 26, 2020 •

edited

Loading