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

types_escaping_snapshot cleanup finalization #84570

Open
Mark-Simulacrum opened this issue Apr 26, 2021 · 4 comments
Open

types_escaping_snapshot cleanup finalization #84570

Mark-Simulacrum opened this issue Apr 26, 2021 · 4 comments
Labels
C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@Mark-Simulacrum
Copy link
Member

#83185 deleted types_escaping_snapshot in compiler/rustc_infer/src/infer/type_variable.rs, but there are several comments remaining referencing that function, some of which seem to indicate no longer necessary operations. This code, to my knowledge, is pretty finicky, so we will want to be careful, but there may be some performance (or at least cleanup) wins left there.

cc @jyn514

@Mark-Simulacrum Mark-Simulacrum added C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Apr 26, 2021
@jyn514
Copy link
Member

jyn514 commented Apr 26, 2021

I'm not sure what's going on with that code. The comment on impl sv::SnapshotVecDelegate for Delegate { says

// In fact, we could almost just remove the
// SnapshotVec entirely, except that we would have to
// reproduce some of its logic, since we want to know which
// type variables have been instantiated since the snapshot
// was started, so we can implement types_escaping_snapshot.

but when I try to remove the impl I get lots of errors:

error[E0277]: the trait bound `type_variable::Delegate: SnapshotVecDelegate` is not satisfied
   --> compiler/rustc_infer/src/infer/type_variable.rs:319:10
    |
319 |     ) -> sv::SnapshotVec<Delegate, &mut Vec<TypeVariableData>, &mut InferCtxtUndoLogs<'tcx>> {
    |          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ the trait `SnapshotVecDelegate` is not implemented for `type_variable::Delegate`
    | 
   ::: /home/joshua/.local/lib/cargo/registry/src/github.com-1ecc6299db9ec823/ena-0.14.0/src/snapshot_vec.rs:111:8
    |
111 |     D: SnapshotVecDelegate,
    |        ------------------- required by this bound in `SnapshotVec`

including from functions that are still used, like values().

The comment was added in 57a593f - @nikomatsakis do any obvious cleanups come to mind now that types_escaping_snapshot no longer exists?

@jyn514
Copy link
Member

jyn514 commented Apr 26, 2021

I did remove

self.undo_log.push(Instantiate { vid });
without any test failures. Maybe self.undo_log can be removed altogether?

@nikomatsakis
Copy link
Contributor

Hmm

@nikomatsakis
Copy link
Contributor

I don't think you can remove the undo_log, that's needed for probes and other things, but I might be forgetting something. There might be some simplifications possible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-cleanup Category: PRs that clean code up or issues documenting cleanup. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

3 participants