Add snapshotting to ObligationForest

[soltanmm]

Adds snapshotting to ObligationForest. The choice of using per-node vecs instead of additional tables or duplication was made under the assumption that if some node was getting hit with a snapshot+rollback, it'd probably get hit with another snapshot in the near future, and that we'd rather branch on superfluous nodes than (re)allocate some number of times at each snapshot. I haven't actually written the alternatives to test the potential performance difference though.

I think I recall a <1% drop in runtime performance with this on top of the previous incarnation of ObligationForest on libsyntax at cursory glance. Can check again if desired.

There are several FIXMEs sprinkled around. Mostly miscellaneous opportunities to perhaps optimize a bit.

r? @nikomatsakis (I think?)
cc @jroesch

EDIT: Needs tidying.

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @nikomatsakis (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[soltanmm]

(sorry - I had to rebase because of policies and incorrect e-mail addresses on commits and whatnot; I don't think anyone was in the process of commenting on the commits or anything anyway, though, so, shouldn't be a problem, jah?)

[nikomatsakis]

Nit: This comment is funny, but you could also say "but we expect each snapshot to be consumed exactly once".

[nikomatsakis]

@soltanmm I'm reading now. I initially thought that more complex snapshotting in the O.F. was not necessary, because we did not process obligations (and hence did not compress) while a snapshot was active. Is that not true?

That said, I'm happy to see you've attempted something more aggressive, as I figured that sooner or later we'd have to add better snapshoting. I was planning on using a WAM-like technique -- which I think is sort of roughly what you've done, actually. I have to go review just what the WAM does here and compare it to the approach that you took. My main memory is that when popping contexts in a snapshot, you don't actually pop them, you keep the "stack pointer" at the end so as to avoid overwriting things that you may need to restore later. This might allows us to avoid the enumeration in Snapshot that you added -- have to think on it. :)

[nikomatsakis]

OK, I understand better what you did. Makes sense, though I'm not sure what I think about maintaining e.g. a stack of states per node. I have to think about it. I also want to refresh my memory on what the WAM does in similar situations.

I would be interested though to see the refreshed perf numbers you mentioned, since one of my concerns about this approach is that it makes the "baseline" case slower -- but then again since we're not really pushing many snapshots, I guess it's not quite as interesting as it might otherwise be.

[soltanmm]

[...] because we did not process obligations (and hence did not compress) while a snapshot was active. Is that not true?

Oh. For some reason I thought that the compression not happening during snapshots was undesirable. What I wrote followed from that being desired and not thinking too much. :-)

I'm not sure what I think about maintaining e.g. a stack of states per node.

Aye, that was done to avoid any extra allocations when not simultaneously snapshotting and processing obligations in the least-thinking-required-way.

A stack of vecs of nodes w/ their parent pointers having choices between the current snapshot and the nth previous and identity backpointers (to keep track of outstanding dependency obligations across snapshots) would've had that property too (at the cost of complexity, methinks).

Or just the single vec, but with node success status calculated by making a backward pass through the vec (with some second vec [or additional node fields] recording what's already been reported or not at what snapshot) instead of having that information explicitly present in the structure.

[...] but then again since we're not really pushing many snapshots, I guess it's not quite as interesting as it might otherwise be.

I'll grab some numbers again, but, yeah I'd be surprised at any significant decrease - the enum'd NodeStateSnapshots was made to make the difference in behavior with what's already there as uninteresting as possible (it doesn't allocate until both a snapshot has been made and a node's state has updated in that snapshot, which was a disallowed case before enforced by assert!s). EDIT: Oops, forgot about the extra Vec in compress.

That all said, I think you're right on being iffy on the one-stack-per node. Want it differently?

[soltanmm]

LD_LIBRARY_PATH= ./x86_64-unknown-linux-gnu/stage3/bin/rustc -Z time-passes 97.24s user 1.60s system 99% cpu 1:39.00 total (before)
vs.
LD_LIBRARY_PATH= ./x86_64-unknown-linux-gnu/stage3/bin/rustc -Z time-passes 99.58s user 1.76s system 99% cpu 1:41.47 total (after)

That was a little worse than I thought. I'll just do the not-as-lazy-way with a single vec some time later tonight and chuck it in tomorrow.

EDIT: Clarification: those numbers were the most favorable comparison, the rest were worse.

[soltanmm]

Updated to just use the single vec. A few cursory runs on libsyntax gave me the impression that whatever difference in performance there is with the baseline is below noise level. The current approach still emulates a sort of stack-of-states per nodes, but without using Vecs.

Left a form of compression in (it was a little less complicated to think about that way).

[soltanmm]

@nikomatsakis It just occurred to me that there was a possible gap in communication on my end now that I've actually reviewed what a WAM is. Did you actually really want something more like what's described here? (EDITEDIT: specifically having a separate 'trail' for the whole structure)

EDIT: I'll note that right now taking a snapshot then adding nothing then taking another snapshot won't work as written; the enumeration is required (and would need to be added back in, so, probably not mergeable at the moment).

[nikomatsakis]

@soltanmm sorry for being incommunicado. Been a crazy week. Let me catch up on what you've done in the meantime. :)

[nikomatsakis]

OK, I think I mostly grok what's going on here. I was thinking earlier that if I were going to add full snapshotting to the OF, I would probably do so by using a similar strategy to what we use elsewhere. That is, I would have an undo vector, and I would have snapshots being an index into that undo vector. When you start a snapshot, you would push an entry into the vector, so by checking if the vector is empty, you can test whether you are in a snapshot or not. The main data structure always stays at the "tip", and to commit a snapshot, you just clear the vector -- but to unroll one, you pop things off and undo them one at a time. See e.g. SnapshotVector for an example.

Following this approach, one could do something like:

• when an item state changes, you mark the transition in the undo vector
  • I probably wouldn't record minor things like subtracting from the number of pending children; instead, when you reverse a state change, you would go and increment the number of pending children back
• compress can still operate as normal, but everything that gets removed from the vector would instead be moved to the undo vector, along with its original index (or, you must move a list of "removed things" and "indices they were at")
  • when reversing a compression step, you would re-insert those items, making room as needed

Anyway, this feels like it might be simpler than what you've done here --- but maybe not. What do you think? Do you understand what I'm trying to describe?

I'm still unsure whether to we will ever want this sort of transactional support in the OF though. Every scenario I can come up with where I would want it would, I think, be better served by making a separate fulfillment context for processing locally. But I'm not sure.

[soltanmm]

@nikomatsakis No worries! And yep, gotcha. Undo list would probably be simpler sans rollbacks (I was really trying to avoid thinking much about rollbacks), so, h'okay. Will make another pass. EDIT: And I think I get why you wanted to keep as close to the baseline as possible (about preferring to spin off a new fulfillment context and whatnot).

[nikomatsakis]

On Thu, Jan 21, 2016 at 09:24:31PM -0800, Masood Malekghassemi wrote:

@nikomatsakis It just occurred to me that there was a possible gap
in communication on my end now that I've actually reviewed what a
WAM is. Did you actually really want something more like what's
described here?

I'm not sure, but that is what I meant by the WAM. I think perhaps
something like that is not needed though, given our current evaluation
strategy (which is less general than prolog).

[soltanmm]

Closing for #31175.