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

Issue #56905 #56906

Merged
merged 5 commits into from
Jan 12, 2019
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Next Next commit
Issue 56905
Adding a map to TypeckTables to get the list of all the Upvars
given a closureID. This is help us get rid of the recurring
pattern in the codebase of iterating over the free vars
using with_freevars.
blitzerr committed Jan 9, 2019
commit 47db51eecee943dc2af1adc467d1ff47685e6d09
10 changes: 10 additions & 0 deletions src/librustc/ty/context.rs
Original file line number Diff line number Diff line change
@@ -417,6 +417,12 @@ pub struct TypeckTables<'tcx> {
/// All the existential types that are restricted to concrete types
/// by this function
pub concrete_existential_types: FxHashMap<DefId, Ty<'tcx>>,

/// Given the closure ID this map provides the list of UpvarIDs used by it.
/// The upvarID contains the HIR node ID and it also contains the full path
/// leading to the member of the struct or tuple that is used instead of the
/// entire variable.
pub upvar_list: ty::UpvarListMap<'tcx>,
}

impl<'tcx> TypeckTables<'tcx> {
@@ -441,6 +447,7 @@ impl<'tcx> TypeckTables<'tcx> {
tainted_by_errors: false,
free_region_map: Default::default(),
concrete_existential_types: Default::default(),
upvar_list: Default::default(),
}
}

@@ -741,6 +748,8 @@ impl<'a, 'gcx> HashStable<StableHashingContext<'a>> for TypeckTables<'gcx> {
tainted_by_errors,
ref free_region_map,
ref concrete_existential_types,
ref upvar_list,

} = *self;

hcx.with_node_id_hashing_mode(NodeIdHashingMode::HashDefPath, |hcx| {
@@ -783,6 +792,7 @@ impl<'a, 'gcx> HashStable<StableHashingContext<'a>> for TypeckTables<'gcx> {
tainted_by_errors.hash_stable(hcx, hasher);
free_region_map.hash_stable(hcx, hasher);
concrete_existential_types.hash_stable(hcx, hasher);
upvar_list.hash_stable(hcx, hasher);
})
}
}
2 changes: 2 additions & 0 deletions src/librustc/ty/mod.rs
Original file line number Diff line number Diff line change
@@ -589,6 +589,8 @@ impl<'a, 'gcx> HashStable<StableHashingContext<'a>> for ty::TyS<'gcx> {

pub type Ty<'tcx> = &'tcx TyS<'tcx>;

pub type UpvarListMap<'tcx> = FxHashMap<DefId, Vec<UpvarId>>;

impl<'tcx> serialize::UseSpecializedEncodable for Ty<'tcx> {}
impl<'tcx> serialize::UseSpecializedDecodable for Ty<'tcx> {}

18 changes: 12 additions & 6 deletions src/librustc_typeck/check/upvar.rs
Original file line number Diff line number Diff line change
@@ -122,14 +122,16 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> {
};

self.tcx.with_freevars(closure_node_id, |freevars| {
let mut freevar_list: Vec<ty::UpvarId> = Vec::new();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: I would use Vec::with_capacity -- or, perhaps even better, convert to an iterator:

let upvar_ids: Vec<_> = freevars.iter().map(|freevar| ty::UpvarId {..}).collect();
self.tables
                 .borrow_mut()
                 .upvar_list
                 .insert(closure_def_id, upvar_ids);

for upvar_id in &upvar_ids {
    ... // as before
}

Copy link
Contributor Author

@blitzerr blitzerr Dec 18, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with the with_capacity part to save on the vector reallocation but by using freevars.iter().map(...).collect() etc. we are iterating over the freevars twice (once here and other time in for freevar in freevars {.. ). Wouldn't you say that's less performant ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Potentially. It seems unlikely to matter in practice, so I was more concerned with what "read better".

You could of course do both at once by adding a inspect in the chain:

let upvar_ids: Vec<_> = freevars.iter()
    .map(|freevar| ty::UpvarId {..})
    .inspect(|&upvar_id| {
        let capture_kind = /* as before */;
        self.tables.borrow_mut().insert(upvar_id, capture_kind);
    })
    .collect();

I tend to prefer not to use explicit with_capacity calls and instead use iterators/collector, but either one is fine. Matter of personal preference I suppose.

for freevar in freevars {
let upvar_id = ty::UpvarId {
var_path: ty::UpvarPath {
hir_id : self.tcx.hir().node_to_hir_id(freevar.var_id()),
hir_id: self.tcx.hir().node_to_hir_id(freevar.var_id()),
},
closure_expr_id: LocalDefId::from_def_id(closure_def_id),
};
debug!("seed upvar_id {:?}", upvar_id);
freevar_list.push(upvar_id);

let capture_kind = match capture_clause {
hir::CaptureByValue => ty::UpvarCapture::ByValue,
@@ -149,6 +151,10 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> {
.upvar_capture_map
.insert(upvar_id, capture_kind);
}
self.tables
.borrow_mut()
.upvar_list
.insert(closure_def_id, freevar_list);
});

let body_owner_def_id = self.tcx.hir().body_owner_def_id(body.id());
@@ -166,7 +172,8 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> {
self.param_env,
region_scope_tree,
&self.tables.borrow(),
).consume_body(body);
)
.consume_body(body);

if let Some(closure_substs) = infer_kind {
// Unify the (as yet unbound) type variable in the closure
@@ -240,9 +247,7 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> {
let var_hir_id = tcx.hir().node_to_hir_id(var_node_id);
let freevar_ty = self.node_ty(var_hir_id);
let upvar_id = ty::UpvarId {
var_path: ty::UpvarPath {
hir_id: var_hir_id,
},
var_path: ty::UpvarPath { hir_id: var_hir_id },
closure_expr_id: LocalDefId::from_def_id(closure_def_index),
};
let capture = self.tables.borrow().upvar_capture(upvar_id);
@@ -262,7 +267,8 @@ impl<'a, 'gcx, 'tcx> FnCtxt<'a, 'gcx, 'tcx> {
},
),
}
}).collect()
})
.collect()
})
}
}